Skip to content

Feat/paperless attachement links#1492

Open
szaiser wants to merge 31 commits into
sysadminsmedia:mainfrom
szaiser:feat/paperless-immich-attachement-links
Open

Feat/paperless attachement links#1492
szaiser wants to merge 31 commits into
sysadminsmedia:mainfrom
szaiser:feat/paperless-immich-attachement-links

Conversation

@szaiser

@szaiser szaiser commented May 12, 2026

Copy link
Copy Markdown
Contributor

Compacted conversation---

What type of PR is this?

  • feature

What this PR does / why we need it:

Builds on #1481 to add a Paperless-ngx integration and lay the groundwork for further service integrations (e.g. Immich). Users can drag-drop a Paperless document URL onto an item's attachment zone — the document is stored as an external reference (no file copy) and renders as a rich card with thumbnail, title, correspondent, document type, tags, page count and an open-in-Paperless button.

homebox_paperless_link.mp4

Changes:

  • New endpoint: GET /v1/integrations/{name}/proxy?path={relPath} — generic authenticated reverse-proxy; reads {name}_url / {name}_token from user settings, validates name and path, forwards Authorization: Token {token}
  • Repo: MimeTypePaperlessDocument = "paperless/document" registered in MimeTypeForSourceType()
  • Frontend: integration-adapters.ts registry (ServiceAdapter interface + SERVICE_ADAPTERS); integration-cache.ts Pinia store for URL state and enriched-data cache; Paperless card in AttachmentsList.vue (thumbnail, metadata, ⚠ error badge); drag-drop URL detection in edit.vue; Paperless URL + token fields in profile.vue
  • Tests: 25 unit tests in integration-adapters.test.ts

Design decisions:

  • Proxy is intentionally service-agnostic; credentials never reach the browser. Adding a future service (e.g. Immich) requires one new SERVICE_ADAPTERS entry on the frontend and one line in MimeTypeForSourceType() on the backend — no further structural changes.
  • link/url attachments from feat: add support for external URL attachments #1481 whose host matches the configured Paperless URL are auto-promoted to the rich card at render time — fully backward compatible
  • Registry pattern eliminates all service-specific if/else chains in drop detection, URL classification, and card hydration

Which issue(s) this PR fixes:

Testing

  • ✅ Frontend unit tests: npx vitest run lib/integration-adapters.test.ts — 25/25 pass
  • ✅ Backend tests: go test ./internal/core/services ./internal/data/repo ./app/api/handlers/v1 pass
  • Manual: tested against live Paperless-ngx v2.x — drag-and-drop, card rendering (thumbnail, tags, correspondent), error badge on wrong token, graceful demotion when URL is cleared

@coderabbitai

coderabbitai Bot commented May 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This PR adds Paperless-backed attachment integration, shared outbound HTTP validation, default entity-type bootstrapping, attachment MIME/tracing changes, preference sync helpers, and frontend updates for cards, settings, templates, and parent-based location handling.

Changes

Backend integration proxy and validation

Layer / File(s) Summary
Outbound URL validation
backend/internal/sys/validate/notifier_url.go, backend/internal/sys/validate/notifier_url_test.go, backend/internal/sys/config/conf.go, docs/src/content/docs/en/user-guide/notifiers.mdx
Notifier URL validation now uses shared outbound HTTP(S) checks and dial-time transport enforcement, with matching tests and docs.
Integration card and thumbnail proxy
backend/app/api/handlers/v1/controller.go, backend/app/api/handlers/v1/v1_ctrl_integration_cards.go, backend/app/api/handlers/v1/v1_ctrl_integration_cards_test.go, backend/app/api/routes.go
New handlers and routes fetch Paperless attachment cards and thumbnails through a controller-owned outbound transport.
External attachment URL parsing
backend/app/api/handlers/v1/v1_ctrl_entities_attachments_external.go
External attachment URL parsing now uses the broader URL parser.

Entity defaults and bootstrap

Layer / File(s) Summary
Repository default-entity helpers
backend/internal/data/repo/repo_entity_types.go, backend/internal/data/repo/repo_entity_types_test.go, backend/internal/data/repo/repo_group.go
Adds default entity-type helpers, EnsureDefaults, and transactional group creation with defaults.
Service bootstrap and OIDC logging
backend/internal/core/services/service_group.go, backend/internal/core/services/service_user.go
Group creation uses the repository default-entity path; registration flows retry default setup and hash OIDC subjects in logs.
Test harness shutdown
backend/internal/core/services/main_test.go, backend/internal/data/repo/main_test.go
Consolidates event-bus and client shutdown into cancellable cleanup.

Attachment MIME and trace handling

Layer / File(s) Summary
External-link MIME registry
backend/internal/data/repo/repo_item_attachments.go, backend/internal/data/repo/repo_item_attachments_test.go
Adds Paperless document MIME support and validates external-link MIME types before creation.
Trace redaction
backend/internal/core/services/service_items_attachments.go, backend/internal/core/services/service_items_attachments_external_test.go
Redacts traced external identifiers and expands attachment-add coverage across source types and delete behavior.

Frontend integration UI and data shapes

Layer / File(s) Summary
Attachment cards and locales
frontend/components/Item/AttachmentsList.vue, frontend/locales/en.json, frontend/locales/de.json
Renders integration cards, safe links, or downloads and adds matching UI strings.
Profile integration settings
frontend/pages/profile.vue
Adds Paperless URL/token settings with load/save flow.
Entity create and edit flows
frontend/components/Entity/CreateModal.vue, frontend/pages/item/[id]/index/edit.vue
Validates resolved parent IDs, updates link toast messaging, and narrows open-in-new-tab behavior.
Template field persistence
frontend/lib/template-fields.ts, frontend/components/Template/Card.vue, frontend/components/Template/CreateModal.vue, frontend/pages/template/[id].vue, frontend/pages/item/[id]/index.vue, frontend/lib/api/__test__/user/templates.test.ts
Adds shared template-field helpers and preserves all supported field value variants across create/edit/duplicate flows.
Parent-based location usage
frontend/components/Item/View/table/columns.ts, frontend/components/Item/View/table/data-table-expanded-row.vue, frontend/components/Scanner/AROverlayCard.vue, frontend/composables/use-barcode-detector.ts, frontend/pages/reports/label-generator.vue, frontend/components/Item/View/table/data-table-dropdown.vue, frontend/components/Item/View/ItemChangeDetails.vue, frontend/lib/api/__test__/user/items.test.ts
Switches multiple views and tests to derive location data from the parent relationship.
API contract and fixtures
frontend/lib/api/classes/items.ts, frontend/lib/api/base/base-api.ts, frontend/lib/api/__test__/factories/index.ts, frontend/lib/api/__test__/user/items.test.ts, frontend/lib/api/__test__/user/stats.test.ts, frontend/lib/api/__test__/user/tags.test.ts
Adds the integration-card API type/method, corrects request typing, and updates fixtures/tests for entity type IDs and new fields.
Preference sync extraction
frontend/lib/preferences-utils.ts, frontend/composables/use-preferences.ts, frontend/composables/use-preferences.test.ts
Moves preference sync helpers into a shared module and merges saves into current server settings.
Miscellaneous frontend cleanup
frontend/pages/collection/index/tools.vue
Broader date input typing for a local formatter.

Estimated code review effort: 4 (Complex) | ~75 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant V1Controller
  participant PaperlessService

  Client->>V1Controller: GET /entities/{id}/attachments/integration-cards
  V1Controller->>V1Controller: load user settings (paperless_url/token)
  V1Controller->>PaperlessService: GET document metadata (Token auth)
  PaperlessService-->>V1Controller: document/correspondent/tags
  V1Controller-->>Client: integrationCardOut list (ok or error state)
Loading
sequenceDiagram
  participant User
  participant GroupCreationService
  participant EntityTypeRepository
  participant RetryGoroutine

  User->>GroupCreationService: register/create group
  GroupCreationService->>EntityTypeRepository: EnsureDefaults(groupID)
  EntityTypeRepository-->>GroupCreationService: error (transient)
  GroupCreationService->>RetryGoroutine: schedule async retry (1m timeout)
  GroupCreationService-->>User: bootstrap completes anyway
  RetryGoroutine->>EntityTypeRepository: EnsureDefaults(groupID) retry
Loading

Possibly related PRs

Suggested labels: ⬆️ enhancement, review needed, go

Suggested reviewers: tankerkiller125, katosdev, tonyaellie

Poem

A Paperless path now lights the way,
With validated URLs and cards on display.
Defaults bloom twice, then settle in place,
While templates keep every field in the trace.
Parent knows best where locations belong,
And prefs stay in sync, tidy and strong.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 39.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and clearly points to the Paperless attachment-link feature in this PR.
Description check ✅ Passed The description matches the template with PR type, summary, issue reference, and testing details.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot added ⬆️ enhancement New feature or request review needed A review is needed on this PR or Issue go Pull requests that update Go code labels May 12, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (6)
frontend/composables/use-preferences.test.ts (1)

38-43: 💤 Low value

Consider using valid theme values in test data.

Line 39 uses theme: "dark" as never, which suggests "dark" may not be a valid DaisyTheme value. Using as never bypasses type checking and reduces test quality.

Recommend either:

  1. Use a valid theme value from the DaisyTheme type (e.g., "homebox")
  2. If "dark" is actually valid, remove the as never assertion

Note: Line 67 in a later test uses theme: "dark" without as never, which is inconsistent.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/composables/use-preferences.test.ts` around lines 38 - 43, The test
uses an invalid cast "theme: \"dark\" as never" which hides type errors; update
the test in use-preferences.test.ts to supply a valid DaisyTheme or remove the
cast: change the prefs object (currently spread from DEFAULT_PREFERENCES) passed
to buildSyncedSettings(SYNC_ALL) so theme is either a real DaisyTheme value
(e.g., "homebox" or whatever is defined in the DaisyTheme union) or, if "dark"
is valid, drop the `as never` and use `theme: "dark"`. Ensure consistency with
the later test that already uses `theme: "dark"` without a cast and keep
references to DEFAULT_PREFERENCES, buildSyncedSettings, and SYNC_ALL unchanged.
frontend/composables/preferences-utils.ts (1)

89-101: 💤 Low value

Consider improving type safety instead of using as never.

The as never assertion on line 97 bypasses TypeScript's type checking entirely, which could hide runtime type errors if the server returns unexpected value types for preference keys.

Consider one of these approaches:

  1. Add runtime type validation before assignment
  2. Define a more specific type for the settings parameter that constrains it to valid preference value types
  3. At minimum, add a comment explaining why this assertion is safe

The current implementation works but sacrifices type safety for convenience.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/composables/preferences-utils.ts` around lines 89 - 101,
mergeSyncedSettings currently silences TypeScript by casting settings[key] to
never before assigning to nextPreferences; instead make the assignment type-safe
by either (A) narrowing the settings parameter to a keyed type that maps known
preference keys to their allowed value types (e.g., Record<keyof
LocationViewPreferences, ...>) so you can assign without assertions, or (B)
perform a runtime type-check per key inside mergeSyncedSettings using a small
type-check map/type guard for each preference key (validate settings[key]
matches the expected type) and only then assign to nextPreferences; remove the
`as never` cast and replace it with the validated value (or tighten the function
signature) and keep forEachSyncedPreference usage the same.
frontend/pages/profile.vue (1)

515-520: ⚡ Quick win

Consider adding a password visibility toggle for the API token field.

Users often need to verify they've pasted their token correctly. The token is stored in settings (not as sensitive as a password that's never stored), so adding a show/hide toggle would improve usability without significantly increasing security risk.

This is already implemented for password fields elsewhere in the app (see FormPassword component with toggle). You could either:

  1. Use FormPassword component which has built-in toggle
  2. Add a similar toggle to the token field

Example using FormPassword:

<FormPassword
  v-model="integrationSettings.paperlessToken"
  :label="$t('profile.paperless_token')"
  :placeholder="$t('profile.paperless_token_placeholder')"
/>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/pages/profile.vue` around lines 515 - 520, Replace the current
FormTextField usage for the API token with a field that supports visibility
toggling (either swap to the existing FormPassword component or add the same
toggle behavior) so users can show/hide integrationSettings.paperlessToken when
verifying input; update the component at the FormTextField instance in
profile.vue (the block rendering integrationSettings.paperlessToken with label
profile.paperless_token and placeholder profile.paperless_token_placeholder) to
use FormPassword or mirror its show/hide logic and props.
backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go (1)

41-128: ⚖️ Poor tradeoff

Consider: Rate limiting per user for proxy endpoint.

Since users control the upstream URL, they could abuse this endpoint to:

  1. Hammer a third-party service with requests
  2. Use the proxy as a generic HTTP client
  3. Cause excessive outbound bandwidth usage

While the existing auth middleware provides per-user isolation, consider adding per-user rate limiting for this endpoint specifically.

Example approach:

// In routes.go, wrap the handler with a rate limiter
r.Get("/integrations/{name}/proxy", 
  chain.ToHandlerFunc(
    v1Ctrl.HandleIntegrationProxy(), 
    append(userMW, a.integrationProxyLimiter.middleware)...,
  ),
)

This is lower priority than the timeout/size fixes but worth considering for production hardening.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go` around lines 41 -
128, Add per-user rate limiting to the integration proxy by wrapping the
V1Controller.HandleIntegrationProxy handler with a user-scoped limiter
middleware: implement or reuse an integrationProxyLimiter that keys limits by
the authenticated user ID (services.UseUserCtx(ctx.Context).ID) and attach its
middleware when registering the route (e.g., in routes.go wrap the GET
"/integrations/{name}/proxy" with
chain.ToHandlerFunc(v1Ctrl.HandleIntegrationProxy(), append(userMW,
a.integrationProxyLimiter.middleware)...)); ensure the limiter runs before the
handler and returns a proper 429/Retry-After response when the per-user quota is
exceeded.
frontend/components/Item/AttachmentsList.vue (2)

105-107: ⚡ Quick win

Hardcoded service name in i18n string.

The service name "Paperless" is hardcoded in the i18n interpolation. If additional services are added in the future, this will need to be updated manually.

Consider deriving the service name from the attachment's MIME type via the adapter registry:

♻️ Suggested refactor for dynamic service names

Add a helper to get the display name:

+function getServiceDisplayName(attachment: ItemAttachment): string {
+  const adapter = SERVICE_ADAPTERS.find(a => a.mimeType === attachment.mimeType);
+  return adapter ? adapter.name.charAt(0).toUpperCase() + adapter.name.slice(1) : "Service";
+}

Then use it in the template:

 <TooltipContent>
-  {{ $t("components.item.attachments_list.open_in_service", { service: "Paperless" }) }}
+  {{ $t("components.item.attachments_list.open_in_service", { service: getServiceDisplayName(attachment) }) }}
 </TooltipContent>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/components/Item/AttachmentsList.vue` around lines 105 - 107, The
template currently hardcodes the service name "Paperless" in the i18n
interpolation for TooltipContent (key
"components.item.attachments_list.open_in_service"); replace this with a dynamic
display name derived from the attachment's MIME type by calling a helper that
looks up the adapter registry (e.g., create a helper like
getServiceDisplayNameFromMime(mimeType) and use it in the component's computed
properties or methods to pass the resolved service name into $t). Ensure
TooltipContent uses the computed/serviceName value instead of the hardcoded
string so future services are handled automatically.

395-444: ⚖️ Poor tradeoff

Silent error swallowing in enrichment sub-fetches.

The correspondent, document_type, and tag fetches use .catch(() => {}) to silently swallow errors (lines 407, 423, 443). This is graceful degradation — if a related entity fetch fails, the main document data is still shown with partial enrichment.

This is likely intentional, but consider whether users should be notified when partial data couldn't be loaded (e.g., "Some metadata unavailable"). Current behavior shows incomplete data without indication.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/components/Item/AttachmentsList.vue` around lines 395 - 444, The
sub-fetches for correspondent, document_type, and tags in AttachmentsList.vue
currently swallow errors with .catch(() => {}) which hides partial-enrichment
failures; replace those empty catches by capturing the error (e) and (1) logging
it (console.error or a logger) and (2) marking a shared flag (e.g.,
enrichmentFailed or push to enrichmentErrors) accessible alongside
doc/tagResults/jobs so the UI can render a small “Some metadata unavailable”
notice when any enrichmentFailed is true; update the code paths that push to
jobs (the correspondent/document_type/tag fetch chains) to set this flag on
error and ensure the component renders the notification when
enrichmentErrors.length>0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 104-109: Replace use of http.DefaultClient in the integration
proxy handler with a dedicated HTTP client that enforces a timeout and sane
transport settings: add a package-level variable (e.g., proxyHTTPClient)
configured with Timeout: 30*time.Second and a Transport that sets MaxIdleConns,
IdleConnTimeout, and DisableCompression as suggested, ensure the package imports
time, and update the call in the handler (where resp, err :=
http.DefaultClient.Do(req)) to use proxyHTTPClient.Do(req) instead.
- Around line 124-126: The handler currently calls io.Copy(w, resp.Body) without
bounds (see w.WriteHeader and io.Copy usage) which can allow huge upstream
responses; wrap resp.Body with io.LimitReader (or use io.CopyN) and enforce a
hard size limit (e.g. const maxRespSize = 10 << 20) when copying to w, detect if
the upstream exceeds that limit and return a safe error/status (e.g. write
http.StatusBadGateway or http.StatusRequestEntityTooLarge and a small error
body) instead of streaming unlimited data; update the code around the resp.Body
copy to use io.LimitReader(resp.Body, maxRespSize+1) and check if bytesCopied >
maxRespSize to handle the overflow case.

In `@backend/internal/core/services/service_items_attachments_external_test.go`:
- Around line 37-44: The test data in the knownSources variable still contains
an Immich entry even though Immich was removed; update the knownSources slice in
service_items_attachments_external_test.go by removing the {"immich",
"1df4f848-dead-beef-cafe-123456789abc"} element (or, if Immich is intentionally
kept, update the PR summary to state that Immich support remains) so the test
data matches the intended adapter registry state; edit the knownSources
declaration to only include the remaining valid sources (e.g., "paperless" and
"link") and run the tests.

In `@frontend/components/Item/AttachmentsList.vue`:
- Around line 351-363: The function describeRequestError returns hardcoded
English messages; replace them with i18n lookups using the
component/localization instance (e.g., $t) and add corresponding keys
(components.item.attachments_list.errors.auth_failed, .request_failed with
{status}, and .service_unreachable with {baseUrl}) to your locale files (e.g.,
frontend/locales/en.json); update describeRequestError to call $t(...) for the
401/403 branch, the generic HTTP branch (passing status), and the network-level
branch (passing baseUrl) so all user-facing strings are translatable.

In `@frontend/lib/integration-adapters.test.ts`:
- Around line 40-42: The extractPaperlessDocId implementation silently falls
back to pattern-only matching when parsing the configured baseUrl fails; update
the catch block in extractPaperlessDocId to emit a developer-facing warning
(e.g., console.warn or the app logger.warn) that includes the invalid baseUrl
and the parse error, and add a small validation helper (e.g.,
validatePaperlessBaseUrl) to run at configuration time to validate/normalize the
baseUrl and surface errors earlier instead of silently relying on heuristic
pattern matching.

In `@frontend/pages/item/`[id]/index/edit.vue:
- Around line 91-102: The loadIntegrationSettings function silently returns when
api.user.getSettings() fails or data?.item is missing; update
loadIntegrationSettings to handle errors by logging the error (e.g.,
console.error or process logger) and surface a user-facing notification/toast so
users know settings failed to load, while preserving the existing fallback to
empty integrationSettings; specifically catch or check for error from
api.user.getSettings(), log the error and call the component's
notification/toast method (or emit an event) before returning, and also validate
data.item before casting to Record<string, unknown> (refer to
loadIntegrationSettings, api.user.getSettings, and integrationSettings).
- Around line 414-419: Replace the hardcoded `${serviceName} linked` toast with
a translatable string: use the i18n key (e.g. "items.toast.service_linked") and
call t with a replacement object so the service name (from
classified.adapter.name → serviceName) is injected, then pass that translated
string into toast.success; ensure the locale files include the "{service}
linked" entry and update the toast invocation near the classified handling code
that computes serviceName.
- Around line 40-46: Remove the unused Immich fields from the reactive
integrationSettings object: delete immich_url and immich_token so
integrationSettings only contains paperless_url and paperless_token; also remove
any code that assigns loaded settings into integrationSettings. Leave
classifyDroppedUrl and SERVICE_ADAPTERS unchanged (they only use Paperless), and
ensure no other references to integrationSettings. This will eliminate vestigial
Immich config while keeping the Paperless integration intact.

---

Nitpick comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 41-128: Add per-user rate limiting to the integration proxy by
wrapping the V1Controller.HandleIntegrationProxy handler with a user-scoped
limiter middleware: implement or reuse an integrationProxyLimiter that keys
limits by the authenticated user ID (services.UseUserCtx(ctx.Context).ID) and
attach its middleware when registering the route (e.g., in routes.go wrap the
GET "/integrations/{name}/proxy" with
chain.ToHandlerFunc(v1Ctrl.HandleIntegrationProxy(), append(userMW,
a.integrationProxyLimiter.middleware)...)); ensure the limiter runs before the
handler and returns a proper 429/Retry-After response when the per-user quota is
exceeded.

In `@frontend/components/Item/AttachmentsList.vue`:
- Around line 105-107: The template currently hardcodes the service name
"Paperless" in the i18n interpolation for TooltipContent (key
"components.item.attachments_list.open_in_service"); replace this with a dynamic
display name derived from the attachment's MIME type by calling a helper that
looks up the adapter registry (e.g., create a helper like
getServiceDisplayNameFromMime(mimeType) and use it in the component's computed
properties or methods to pass the resolved service name into $t). Ensure
TooltipContent uses the computed/serviceName value instead of the hardcoded
string so future services are handled automatically.
- Around line 395-444: The sub-fetches for correspondent, document_type, and
tags in AttachmentsList.vue currently swallow errors with .catch(() => {}) which
hides partial-enrichment failures; replace those empty catches by capturing the
error (e) and (1) logging it (console.error or a logger) and (2) marking a
shared flag (e.g., enrichmentFailed or push to enrichmentErrors) accessible
alongside doc/tagResults/jobs so the UI can render a small “Some metadata
unavailable” notice when any enrichmentFailed is true; update the code paths
that push to jobs (the correspondent/document_type/tag fetch chains) to set this
flag on error and ensure the component renders the notification when
enrichmentErrors.length>0.

In `@frontend/composables/preferences-utils.ts`:
- Around line 89-101: mergeSyncedSettings currently silences TypeScript by
casting settings[key] to never before assigning to nextPreferences; instead make
the assignment type-safe by either (A) narrowing the settings parameter to a
keyed type that maps known preference keys to their allowed value types (e.g.,
Record<keyof LocationViewPreferences, ...>) so you can assign without
assertions, or (B) perform a runtime type-check per key inside
mergeSyncedSettings using a small type-check map/type guard for each preference
key (validate settings[key] matches the expected type) and only then assign to
nextPreferences; remove the `as never` cast and replace it with the validated
value (or tighten the function signature) and keep forEachSyncedPreference usage
the same.

In `@frontend/composables/use-preferences.test.ts`:
- Around line 38-43: The test uses an invalid cast "theme: \"dark\" as never"
which hides type errors; update the test in use-preferences.test.ts to supply a
valid DaisyTheme or remove the cast: change the prefs object (currently spread
from DEFAULT_PREFERENCES) passed to buildSyncedSettings(SYNC_ALL) so theme is
either a real DaisyTheme value (e.g., "homebox" or whatever is defined in the
DaisyTheme union) or, if "dark" is valid, drop the `as never` and use `theme:
"dark"`. Ensure consistency with the later test that already uses `theme:
"dark"` without a cast and keep references to DEFAULT_PREFERENCES,
buildSyncedSettings, and SYNC_ALL unchanged.

In `@frontend/pages/profile.vue`:
- Around line 515-520: Replace the current FormTextField usage for the API token
with a field that supports visibility toggling (either swap to the existing
FormPassword component or add the same toggle behavior) so users can show/hide
integrationSettings.paperlessToken when verifying input; update the component at
the FormTextField instance in profile.vue (the block rendering
integrationSettings.paperlessToken with label profile.paperless_token and
placeholder profile.paperless_token_placeholder) to use FormPassword or mirror
its show/hide logic and props.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 3a8cf4a1-03dd-4a60-8ea5-c76e7c97c8d2

📥 Commits

Reviewing files that changed from the base of the PR and between e5b0fe2 and 4cf2532.

⛔ Files ignored due to path filters (1)
  • backend/go.sum is excluded by !**/*.sum
📒 Files selected for processing (16)
  • backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go
  • backend/app/api/routes.go
  • backend/internal/core/services/service_items_attachments_external_test.go
  • backend/internal/data/repo/repo_item_attachments.go
  • backend/internal/data/repo/repo_item_attachments_test.go
  • docs/todo-integration-features.md
  • frontend/components/Item/AttachmentsList.vue
  • frontend/composables/preferences-utils.ts
  • frontend/composables/use-preferences.test.ts
  • frontend/composables/use-preferences.ts
  • frontend/lib/integration-adapters.test.ts
  • frontend/lib/integration-adapters.ts
  • frontend/locales/en.json
  • frontend/pages/item/[id]/index/edit.vue
  • frontend/pages/profile.vue
  • frontend/stores/integration-cache.ts

Comment thread backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go Outdated
Comment thread backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go Outdated
Comment thread frontend/components/Item/AttachmentsList.vue Outdated
Comment thread frontend/lib/integration-adapters.test.ts Outdated
Comment thread frontend/pages/item/[id]/index/edit.vue Outdated
Comment thread frontend/pages/item/[id]/index/edit.vue Outdated
szaiser pushed a commit to szaiser/homebox that referenced this pull request May 12, 2026
…dia#1492

- backend: replace http.DefaultClient with dedicated proxyHTTPClient
  (30s timeout, bounded transport) to prevent upstream hangs (critical)
- backend: cap proxy response at 10 MB via io.LimitReader to prevent
  memory exhaustion from unbounded upstream responses (major)
- frontend: remove vestigial immich_url / immich_token fields from
  integrationSettings in edit.vue; these were never used after the
  Immich removal (minor)
- frontend: add console.warn in loadIntegrationSettings on API failure
  instead of silently returning (minor)
- frontend: replace hardcoded service-linked toast string with i18n key
  items.toast.service_linked (minor)
- frontend: replace hardcoded English strings in describeRequestError
  with i18n keys under components.item.attachments_list.errors (minor)
- frontend: add console.warn in integration-adapters.ts catch block
  when baseUrl fails to parse (minor)
- locales: add items.toast.service_linked and
  components.item.attachments_list.errors.{auth_failed,request_failed,
  service_unreachable} keys to en.json
szaiser pushed a commit to szaiser/homebox that referenced this pull request May 12, 2026
- Remove MimeTypeImmichAsset constant from repo_item_attachments.go
- Remove immich from externalLinkMimeTypes slice
- Remove immich case from MimeTypeForSourceType()
- Remove immich test block from repo_item_attachments_test.go
  (TestMimeTypeForSourceType and externalLinkMimeTypeCases)
- Remove immich entry from knownSources in
  service_items_attachments_external_test.go

Resolves CodeRabbit review comment on PR sysadminsmedia#1492 (immich inconsistency).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go (1)

35-52: ⚡ Quick win

Update the godoc or implement per-integration auth scheme support.

Line 114 hardcodes Authorization: Token <token> for all integrations. This contradicts the godoc claim on lines 41–43 that "adding a new integration only requires a Vue component and a settings entry — no new Go code." Paperless uses Token auth, but Immich uses x-api-key, and most modern APIs use Authorization: Bearer. The frontend adapter registry is already extensible, but the backend lacks a way to specify per-integration auth schemes.

Either:

  1. Store auth scheme in settings ({name}_auth_scheme, default "Token"), or
  2. Add a backend per-integration registry mapping names to auth scheme handlers

Choose one to make the endpoint's actual behavior match its documented behavior, especially important for multi-tenancy/user data isolation considerations with external service credentials.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go` around lines 35 -
52, The handler HandleIntegrationProxy currently always sets "Authorization:
Token <token>" using the {name}_token setting; update it to read an optional
{name}_auth_scheme setting (default "Token") and use that to build the outgoing
auth header (support at least "Token", "Bearer", and "x-api-key" schemes) before
sending the proxied request, so the code that retrieves {name}_token and sets
the header is replaced with logic that switches on authScheme and sets either
Authorization: Token <token>, Authorization: Bearer <token>, or X-API-Key:
<token> accordingly; also update the godoc to reflect that integrations may
specify an auth scheme via settings and/or validate unknown schemes with a 400
or fall back to default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 138-144: Replace the current truncation check that treats n ==
maxResponseSize as truncated by reading one extra byte: use
io.LimitReader(resp.Body, maxResponseSize+1) (instead of maxResponseSize) when
copying to w, then treat truncation only when n > maxResponseSize; keep logging
via log.Warn().Str("integration", name).Int64("bytes", n).Msg(...) and preserve
the existing copyErr handling. This ensures a true positive truncation detection
for the variables n, copyErr, resp.Body and maxResponseSize in the integration
proxy response copy block.
- Around line 92-108: The handler currently proxies user-provided
settings[name+"_url"] without IP filtering, hardcodes the auth scheme as "Token
"+token, and misdetects truncation; fix by (1) adding a custom DialContext to
proxyHTTPClient that resolves the target hostname and rejects connections to
loopback, link-local, unspecified and RFC1918/private ranges (or enforce
HTTPS/host allow-list) before dialing; reference proxyHTTPClient and the
upstream variable to locate where to apply the DialContext and validation; (2)
make the auth scheme configurable by reading settings[name+"_auth_scheme"]
(fallback to "Token") instead of always using "Token "+token so integrations can
use Bearer/Basic/x-api-key; reference the code that sets the Authorization
header; and (3) detect response truncation correctly by reading with
io.LimitReader(resp.Body, maxResponseSize+1) and checking if bytesRead >
maxResponseSize (reference maxResponseSize and the io.LimitReader usage).

In `@frontend/pages/item/`[id]/index/edit.vue:
- Around line 89-99: The loadIntegrationSettings function should avoid logging
the raw error object and must validate setting types before assigning to
integrationSettings; change the error logging to a generic message (e.g.,
"Failed to load integration settings") and, if you need details, log only safe
fields like error?.message, and ensure api.user.getSettings() errors are not
printing full payloads. For assignments to integrationSettings.paperless_url and
paperless_token, check the type of settings.paperless_url and
settings.paperless_token (e.g., typeof === "string") before using them,
otherwise assign an empty string or a safe default; also treat data.item
defensively (ensure it's an object) when reading values from
api.user.getSettings.

---

Nitpick comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 35-52: The handler HandleIntegrationProxy currently always sets
"Authorization: Token <token>" using the {name}_token setting; update it to read
an optional {name}_auth_scheme setting (default "Token") and use that to build
the outgoing auth header (support at least "Token", "Bearer", and "x-api-key"
schemes) before sending the proxied request, so the code that retrieves
{name}_token and sets the header is replaced with logic that switches on
authScheme and sets either Authorization: Token <token>, Authorization: Bearer
<token>, or X-API-Key: <token> accordingly; also update the godoc to reflect
that integrations may specify an auth scheme via settings and/or validate
unknown schemes with a 400 or fall back to default.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b20defff-c331-4d61-90b5-90df83d23fbb

📥 Commits

Reviewing files that changed from the base of the PR and between 4cf2532 and 01fbe70.

📒 Files selected for processing (5)
  • backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go
  • frontend/components/Item/AttachmentsList.vue
  • frontend/lib/integration-adapters.ts
  • frontend/locales/en.json
  • frontend/pages/item/[id]/index/edit.vue
✅ Files skipped from review due to trivial changes (1)
  • frontend/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
  • frontend/lib/integration-adapters.ts
  • frontend/components/Item/AttachmentsList.vue

Comment thread backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go Outdated
Comment thread backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go Outdated
Comment thread frontend/pages/item/[id]/index/edit.vue Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
backend/internal/data/repo/repo_item_attachments.go (1)

98-110: Ensure path validation occurs at the API handler layer.

The repository correctly stores the external ID without validation (appropriate for the data access layer). Per the PR objectives, ensure the API handler validates both the name and path parameters before calling repository methods to prevent injection or traversal attacks.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/internal/data/repo/repo_item_attachments.go` around lines 98 - 110,
The repo currently accepts external IDs without validation (e.g.,
MimeTypeForSourceType in repo_item_attachments.go), so add explicit validation
in the API handler(s) that call the repository attachment methods: check both
name and path are non-empty, within length limits, match an allowed character
set (or whitelist), normalize with filepath.Clean and then reject values that
are absolute (start with "/") or contain path traversal (cleaned starts with
".." or contains ".." segments or differs from a safe relative representation),
and return a 4xx error before invoking any repo methods; implement this
validation in the handler(s) that parse the incoming request and call the repo
attachment functions so the data layer continues to accept raw IDs but only
after the API layer has validated them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@backend/internal/data/repo/repo_item_attachments.go`:
- Around line 98-110: The repo currently accepts external IDs without validation
(e.g., MimeTypeForSourceType in repo_item_attachments.go), so add explicit
validation in the API handler(s) that call the repository attachment methods:
check both name and path are non-empty, within length limits, match an allowed
character set (or whitelist), normalize with filepath.Clean and then reject
values that are absolute (start with "/") or contain path traversal (cleaned
starts with ".." or contains ".." segments or differs from a safe relative
representation), and return a 4xx error before invoking any repo methods;
implement this validation in the handler(s) that parse the incoming request and
call the repo attachment functions so the data layer continues to accept raw IDs
but only after the API layer has validated them.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 9e9b5dc7-a03c-42fb-afb7-809396468b4a

📥 Commits

Reviewing files that changed from the base of the PR and between 01fbe70 and 7cf7d3e.

📒 Files selected for processing (4)
  • backend/internal/core/services/service_items_attachments_external_test.go
  • backend/internal/data/repo/repo_item_attachments.go
  • backend/internal/data/repo/repo_item_attachments_test.go
  • frontend/composables/use-preferences.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • frontend/composables/use-preferences.test.ts
  • backend/internal/core/services/service_items_attachments_external_test.go

szaiser pushed a commit to szaiser/homebox that referenced this pull request May 12, 2026
- Add ssrfSafeDialContext: custom DialContext that rejects loopback,
  link-local (incl. AWS/GCP/Azure IMDS 169.254.169.254), RFC1918,
  shared (RFC6598), and ULA ranges; public hostnames are unrestricted.
  Both literal IPs and DNS-resolved names are validated before dialing.
- Add blockedCIDRs package-level var (lazy-initialised via IIFE).
- Add checkBlockedIP helper.
- Wire ssrfSafeDialContext into proxyHTTPClient Transport.DialContext.
- Validate baseURL scheme (http:// or https://) before proxying.
- Fix truncation false-positive: buffer with LimitReader(body, max+1),
  detect len(buf) > max and return 502 before writing any headers,
  so callers never receive a partial 200 with invalid JSON.
- Reject known-oversized responses via Content-Length check upfront.
- Fix loadIntegrationSettings (edit.vue): drop raw error from console.warn;
  use typeof guards instead of unsafe string casts.

Resolves CodeRabbit review comments on PR sysadminsmedia#1492.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 194-200: The outbound request is created with http.NewRequest
which loses the incoming request's context; replace it with
http.NewRequestWithContext(req.Context(), http.MethodGet, upstream, nil) (or the
handler's ctx variable) so client cancellations and deadlines propagate
upstream, then continue to set the Authorization header and call
proxyHTTPClient.Do(req) as before and preserve the existing error handling via
validate.NewRequestError.
- Around line 200-203: The upstream URL is logged verbatim in the error path,
which can leak query strings or credentials; before calling log.Err(...) replace
the raw upstream with a sanitized version (e.g., parse upstream with net/url,
clear u.User and u.RawQuery/Fragment, then build a string like u.Scheme + "://"
+ u.Host + u.Path or simply u.Host + u.Path) and use that sanitized value in the
.Str("upstream", ...) field; implement this directly where
proxyHTTPClient.Do(req) error is handled (the log.Err(err).Str("integration",
name).Str("upstream", upstream).Msg(...) call) or extract the logic into a small
helper function (e.g., sanitizeUpstreamURL) and call it before logging.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0dfd9b0a-6960-425c-9d79-69e51c6182ba

📥 Commits

Reviewing files that changed from the base of the PR and between 7cf7d3e and 7c01b94.

📒 Files selected for processing (2)
  • backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go
  • frontend/pages/item/[id]/index/edit.vue
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/pages/item/[id]/index/edit.vue

Comment thread backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go Outdated
Comment thread backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go Outdated
@szaiser

szaiser commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Reviews resumed.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
backend/internal/core/services/service_items_attachments_external_test.go (1)

149-150: ⚡ Quick win

Assert no persistence on rejected source type (security contract).

This test checks the error path, but it does not verify the “before any DB write” guarantee. Add a post-check that the entity still has zero attachments after the call to lock in the allowlist/no-write behavior for untrusted sourceType input.

Suggested test hardening
 func TestEntityService_AttachmentAddExternalLink_UnknownSourceType(t *testing.T) {
 	svc := &EntityService{repo: tRepos}
 	entity := newExternalLinkEntity(t)

 	_, err := svc.AttachmentAddExternalLink(tCtx, entity.ID, "unknown-source", "42", "Unknown", attachment.TypeAttachment)
 	assert.Error(t, err)
+
+	latest, getErr := svc.repo.Entities.GetOneByGroup(tCtx, tCtx.GID, entity.ID)
+	require.NoError(t, getErr)
+	assert.Empty(t, latest.Attachments)
 }

As per coding guidelines, service-layer behavior should be validated in service tests; this strengthens the rejection-path contract in backend/internal/core/services/.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/internal/core/services/service_items_attachments_external_test.go`
around lines 149 - 150, The test calls svc.AttachmentAddExternalLink(tCtx,
entity.ID, "unknown-source", "42", "Unknown", attachment.TypeAttachment) and
asserts an error but doesn't verify no DB write occurred; after the call add a
retrieval of the entity via the same service/repo path (e.g., call svc.Get or
svc.EntityByID used elsewhere) or query the attachments for entity.ID and assert
length is zero to ensure no attachment was persisted when sourceType is
untrusted, preserving the “no-write on rejected source” contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@backend/internal/core/services/service_items_attachments_external_test.go`:
- Around line 149-150: The test calls svc.AttachmentAddExternalLink(tCtx,
entity.ID, "unknown-source", "42", "Unknown", attachment.TypeAttachment) and
asserts an error but doesn't verify no DB write occurred; after the call add a
retrieval of the entity via the same service/repo path (e.g., call svc.Get or
svc.EntityByID used elsewhere) or query the attachments for entity.ID and assert
length is zero to ensure no attachment was persisted when sourceType is
untrusted, preserving the “no-write on rejected source” contract.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 16c4eef5-1245-416f-8352-e998fd3ca406

📥 Commits

Reviewing files that changed from the base of the PR and between 7c01b94 and 1ed1bc6.

📒 Files selected for processing (11)
  • backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go
  • backend/internal/core/services/service_items_attachments_external_test.go
  • backend/internal/data/repo/repo_item_attachments.go
  • backend/internal/data/repo/repo_item_attachments_test.go
  • frontend/components/Item/AttachmentsList.vue
  • frontend/composables/use-preferences.test.ts
  • frontend/lib/integration-adapters.test.ts
  • frontend/lib/integration-adapters.ts
  • frontend/pages/item/[id]/index/edit.vue
  • frontend/pages/profile.vue
  • frontend/stores/integration-cache.ts
🚧 Files skipped from review as they are similar to previous changes (8)
  • frontend/pages/profile.vue
  • backend/internal/data/repo/repo_item_attachments.go
  • frontend/stores/integration-cache.ts
  • frontend/composables/use-preferences.test.ts
  • frontend/lib/integration-adapters.ts
  • backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go
  • frontend/components/Item/AttachmentsList.vue
  • backend/internal/data/repo/repo_item_attachments_test.go

@szaiser

szaiser commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

@tankerkiller125 From my perspective, the PR is ready for review. Will you find time to review anytime soon? Let me know if there is any way to assist (refactor, simplify, split, share info, ..)

szaiser pushed a commit to szaiser/homebox that referenced this pull request May 16, 2026
…dia#1492

- backend: replace http.DefaultClient with dedicated proxyHTTPClient
  (30s timeout, bounded transport) to prevent upstream hangs (critical)
- backend: cap proxy response at 10 MB via io.LimitReader to prevent
  memory exhaustion from unbounded upstream responses (major)
- frontend: remove vestigial immich_url / immich_token fields from
  integrationSettings in edit.vue; these were never used after the
  Immich removal (minor)
- frontend: add console.warn in loadIntegrationSettings on API failure
  instead of silently returning (minor)
- frontend: replace hardcoded service-linked toast string with i18n key
  items.toast.service_linked (minor)
- frontend: replace hardcoded English strings in describeRequestError
  with i18n keys under components.item.attachments_list.errors (minor)
- frontend: add console.warn in integration-adapters.ts catch block
  when baseUrl fails to parse (minor)
- locales: add items.toast.service_linked and
  components.item.attachments_list.errors.{auth_failed,request_failed,
  service_unreachable} keys to en.json
szaiser pushed a commit to szaiser/homebox that referenced this pull request May 16, 2026
- Remove MimeTypeImmichAsset constant from repo_item_attachments.go
- Remove immich from externalLinkMimeTypes slice
- Remove immich case from MimeTypeForSourceType()
- Remove immich test block from repo_item_attachments_test.go
  (TestMimeTypeForSourceType and externalLinkMimeTypeCases)
- Remove immich entry from knownSources in
  service_items_attachments_external_test.go

Resolves CodeRabbit review comment on PR sysadminsmedia#1492 (immich inconsistency).
szaiser pushed a commit to szaiser/homebox that referenced this pull request May 16, 2026
- Add ssrfSafeDialContext: custom DialContext that rejects loopback,
  link-local (incl. AWS/GCP/Azure IMDS 169.254.169.254), RFC1918,
  shared (RFC6598), and ULA ranges; public hostnames are unrestricted.
  Both literal IPs and DNS-resolved names are validated before dialing.
- Add blockedCIDRs package-level var (lazy-initialised via IIFE).
- Add checkBlockedIP helper.
- Wire ssrfSafeDialContext into proxyHTTPClient Transport.DialContext.
- Validate baseURL scheme (http:// or https://) before proxying.
- Fix truncation false-positive: buffer with LimitReader(body, max+1),
  detect len(buf) > max and return 502 before writing any headers,
  so callers never receive a partial 200 with invalid JSON.
- Reject known-oversized responses via Content-Length check upfront.
- Fix loadIntegrationSettings (edit.vue): drop raw error from console.warn;
  use typeof guards instead of unsafe string casts.

Resolves CodeRabbit review comments on PR sysadminsmedia#1492.
@szaiser szaiser force-pushed the feat/paperless-immich-attachement-links branch from 1ed1bc6 to 30e18e2 Compare May 16, 2026 08:19

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@frontend/pages/item/`[id]/index/edit.vue:
- Line 835: The external-link gate currently checks attachment.path with
case/whitespace-sensitive startsWith checks; replace that predicate with the
existing URL validator by calling isValidHttpURL on the attachment path (trimmed
and handling null/undefined) so the Tooltip v-if uses
isValidHttpURL(attachment.path ?? '') (or isValidHttpURL((attachment.path ??
'').trim())) instead of the startsWith checks; update the Tooltip condition and
any related external-link action gating to reuse isValidHttpURL to ensure
consistent, case-insensitive, whitespace-tolerant validation.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e8ee0971-91ac-4090-806c-42d93bb3d4b2

📥 Commits

Reviewing files that changed from the base of the PR and between 30e18e2 and 86ba34f.

⛔ Files ignored due to path filters (1)
  • backend/go.sum is excluded by !**/*.sum
📒 Files selected for processing (1)
  • frontend/pages/item/[id]/index/edit.vue

Comment thread frontend/pages/item/[id]/index/edit.vue Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/components/Location/CreateModal.vue (1)

8-15: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Hardcoded strings should be translatable.

Lines 8 and 15 contain hardcoded UI strings ("Type" and "Select type...") that should use $t() for internationalization, consistent with the rest of the component.

🌐 Proposed fix
-        <Label for="location-type-select" class="px-1">Type</Label>
+        <Label for="location-type-select" class="px-1">{{ $t('global.type') }}</Label>
         <select
           id="location-type-select"
           class="w-full rounded-md border bg-background px-3 py-2 text-sm"
           :value="selectedEntityType?.id || ''"
           `@change`="onEntityTypeChanged(($event.target as HTMLSelectElement).value)"
         >
-          <option value="">Select type...</option>
+          <option value="">{{ $t('global.select') }}</option>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/components/Location/CreateModal.vue` around lines 8 - 15, Replace
the two hardcoded UI strings with translatable calls: change the Label text
"Type" to use $t(...) and change the option "Select type..." to use $t(...),
e.g., $t('location.type') and $t('location.selectType') (or your project's
established keys), keeping the same elements (Label, select option) and leaving
the :value and `@change` handler (onEntityTypeChanged, selectedEntityType) intact
so behavior doesn't change.

Source: Coding guidelines

🧹 Nitpick comments (2)
frontend/components/Template/CreateModal.vue (1)

147-159: 💤 Low value

Consider extracting the newTemplateField() factory to a shared utility.

The same factory function with identical logic exists in frontend/pages/template/[id].vue (lines 87-98). While this duplication is minor and localized, extracting it to a shared utility would improve maintainability and ensure consistency if the default field shape changes.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/components/Template/CreateModal.vue` around lines 147 - 159, Extract
the duplicated factory by moving DEFAULT_TIME_VALUE and the newTemplateField()
function (which returns a TemplateField with id: NIL_UUID, name:"", type:"text",
booleanValue:false, numberValue:0, textValue:"", timeValue:DEFAULT_TIME_VALUE)
into a shared utility module (e.g., a new file under frontend/utils or
frontend/lib) and export newTemplateField; then replace the local
implementations in CreateModal.vue and pages/template/[id].vue with an import of
the shared newTemplateField and remove the duplicate definitions, ensuring
NIL_UUID is referenced from the same source or re-exported if needed so both
components use the single shared factory.
backend/app/api/main.go (1)

203-216: middleware.RealIP removal: no breakage found for logging/security; confirm TrustProxy config for rate limiting

  • mid.Logger (backend/internal/web/mid/logger.go) logs only method/path/status/rid and does not read r.RemoteAddr, so request logging won’t be affected by dropping middleware.RealIP.
  • Rate limiting uses extractClientIP(..., trustProxy) (backend/app/api/middleware.go), which trusts X-Real-IP/X-Forwarded-For only when trustProxy is enabled, otherwise it falls back to r.RemoteAddr.
  • Codebase search shows r.RemoteAddr usage in handlers is limited to the limiter/IP-extraction path (no other request/security logic appears to depend on r.RemoteAddr).

Security recommendation: ensure cfg.AuthRateLimit.Options.TrustProxy is set appropriately for your deployment (behind a proxy/load balancer, otherwise rate-limit keys may become the proxy IP).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/api/main.go` around lines 203 - 216, Removed middleware.RealIP
may change how client IPs are seen by rate limiting; confirm and ensure
TrustProxy is configured correctly. Verify that
cfg.AuthRateLimit.Options.TrustProxy is set for deployments behind a
proxy/load-balancer so extractClientIP (used by the rate limiter in
middleware.go) will trust X-Forwarded-For/X-Real-IP when appropriate; if not
behind a proxy leave TrustProxy false so extractClientIP falls back to
r.RemoteAddr. Also review mid.Logger and other middleware (mid.Logger,
middleware.Recoverer) to ensure no other code expects middleware.RealIP and
update configuration or docs to state the required TrustProxy setting for
correct rate-limiting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/internal/core/services/main_test.go`:
- Around line 61-63: The test spawns the event bus with context.Background() and
ignores its error, leaking the goroutine and hiding failures; change to use a
cancellable context (ctx, cancel := context.WithCancel(context.Background()))
and register cancel via t.Cleanup(cancel) so the bus is stopped when the test
ends, run tbus.Run(ctx) in a goroutine but send its returned error on a channel,
and in t.Cleanup wait for that goroutine to exit and fail the test (t.Fatalf or
t.Error) if the run returned a non-nil error so panics/failures aren’t silently
discarded; reference tbus.Run and the anonymous goroutine when making these
changes.

In `@backend/internal/data/repo/repo_entity_types.go`:
- Around line 85-90: Remove the calls to GetDefault from the GetAll method so
GetAll remains a pure read operation: delete the two GetDefault(ctx, gid, ...)
invocations in repo_entity_types.go and revert GetAll to only query and return
existing entity types. Move default-creation responsibility to the group
initialization flow (e.g., add a call from GroupRepository.Create or an explicit
EnsureDefaults(ctx, groupID) invoked during group creation) so defaults are
created once inside a transactional setup path instead of lazily in GetAll;
ensure EnsureDefaults (or the group create flow) calls the same GetDefault logic
and handles errors/concurrency there.

---

Outside diff comments:
In `@frontend/components/Location/CreateModal.vue`:
- Around line 8-15: Replace the two hardcoded UI strings with translatable
calls: change the Label text "Type" to use $t(...) and change the option "Select
type..." to use $t(...), e.g., $t('location.type') and $t('location.selectType')
(or your project's established keys), keeping the same elements (Label, select
option) and leaving the :value and `@change` handler (onEntityTypeChanged,
selectedEntityType) intact so behavior doesn't change.

---

Nitpick comments:
In `@backend/app/api/main.go`:
- Around line 203-216: Removed middleware.RealIP may change how client IPs are
seen by rate limiting; confirm and ensure TrustProxy is configured correctly.
Verify that cfg.AuthRateLimit.Options.TrustProxy is set for deployments behind a
proxy/load-balancer so extractClientIP (used by the rate limiter in
middleware.go) will trust X-Forwarded-For/X-Real-IP when appropriate; if not
behind a proxy leave TrustProxy false so extractClientIP falls back to
r.RemoteAddr. Also review mid.Logger and other middleware (mid.Logger,
middleware.Recoverer) to ensure no other code expects middleware.RealIP and
update configuration or docs to state the required TrustProxy setting for
correct rate-limiting behavior.

In `@frontend/components/Template/CreateModal.vue`:
- Around line 147-159: Extract the duplicated factory by moving
DEFAULT_TIME_VALUE and the newTemplateField() function (which returns a
TemplateField with id: NIL_UUID, name:"", type:"text", booleanValue:false,
numberValue:0, textValue:"", timeValue:DEFAULT_TIME_VALUE) into a shared utility
module (e.g., a new file under frontend/utils or frontend/lib) and export
newTemplateField; then replace the local implementations in CreateModal.vue and
pages/template/[id].vue with an import of the shared newTemplateField and remove
the duplicate definitions, ensuring NIL_UUID is referenced from the same source
or re-exported if needed so both components use the single shared factory.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d595a077-6aa9-4182-86e9-7240ec1b5a55

📥 Commits

Reviewing files that changed from the base of the PR and between b81427c and bf138f1.

⛔ Files ignored due to path filters (2)
  • backend/go.sum is excluded by !**/*.sum
  • frontend/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (38)
  • backend/app/api/main.go
  • backend/app/api/routes.go
  • backend/internal/core/services/main_test.go
  • backend/internal/core/services/service_items_attachments_external_test.go
  • backend/internal/data/repo/repo_entity_templates.go
  • backend/internal/data/repo/repo_entity_types.go
  • frontend/components/Item/AttachmentsList.vue
  • frontend/components/Item/Card.vue
  • frontend/components/Item/CreateModal.vue
  • frontend/components/Item/View/ItemChangeDetails.vue
  • frontend/components/Item/View/table/columns.ts
  • frontend/components/Item/View/table/data-table-expanded-row.vue
  • frontend/components/Location/CreateModal.vue
  • frontend/components/Scanner/AROverlayCard.vue
  • frontend/components/Template/Card.vue
  • frontend/components/Template/CreateModal.vue
  • frontend/composables/use-barcode-detector.ts
  • frontend/composables/use-preferences.ts
  • frontend/lib/api/__test__/factories/index.ts
  • frontend/lib/api/__test__/test-utils.ts
  • frontend/lib/api/__test__/user/items.test.ts
  • frontend/lib/api/__test__/user/tags.test.ts
  • frontend/lib/api/__test__/user/templates.test.ts
  • frontend/lib/api/classes/items.ts
  • frontend/lib/integration-adapters.ts
  • frontend/locales/en.json
  • frontend/package.json
  • frontend/pages/collection/index/entity-types.vue
  • frontend/pages/collection/index/tools.vue
  • frontend/pages/item/[id]/index.vue
  • frontend/pages/item/[id]/index/edit.vue
  • frontend/pages/location/[id]/index/edit.vue
  • frontend/pages/location/[id]/index/index.vue
  • frontend/pages/profile.vue
  • frontend/pages/reports/label-generator.vue
  • frontend/pages/reset-password.vue
  • frontend/pages/template/[id].vue
  • frontend/stores/integration-cache.ts
💤 Files with no reviewable changes (13)
  • frontend/pages/reset-password.vue
  • frontend/pages/collection/index/tools.vue
  • frontend/package.json
  • frontend/pages/item/[id]/index.vue
  • frontend/pages/reports/label-generator.vue
  • frontend/pages/collection/index/entity-types.vue
  • frontend/pages/location/[id]/index/index.vue
  • frontend/pages/template/[id].vue
  • frontend/pages/item/[id]/index/edit.vue
  • frontend/pages/location/[id]/index/edit.vue
  • frontend/stores/integration-cache.ts
  • frontend/pages/profile.vue
  • frontend/lib/integration-adapters.ts
✅ Files skipped from review due to trivial changes (2)
  • frontend/lib/api/classes/items.ts
  • frontend/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (4)
  • backend/app/api/routes.go
  • frontend/composables/use-preferences.ts
  • backend/internal/core/services/service_items_attachments_external_test.go
  • frontend/components/Item/AttachmentsList.vue

Comment thread backend/internal/core/services/main_test.go
Comment thread backend/internal/data/repo/repo_entity_types.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/pages/template/[id].vue (1)

107-111: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve existing field type in edit mapping.

Line 110 hardcodes type to "text", which rewrites persisted field types when saving updates and can corrupt template field semantics.

Proposed fix
       fields: template.value.fields.map(f => ({
         id: f.id,
         name: f.name,
-        type: "text" as const,
+        type: f.type,
         booleanValue: f.booleanValue,
         numberValue: f.numberValue,
         textValue: f.textValue,
         timeValue: f.timeValue,
       })),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/pages/template/`[id].vue around lines 107 - 111, The mapping in
template.value.fields.map currently hardcodes the field type ("type": "text")
which overwrites persisted types; change the mapper to preserve the original
type by using the existing property (e.g., use f.type) when building each field
object in the map so Template editing does not corrupt field semantics (update
the mapping in the code that constructs fields from template.value.fields).
🧹 Nitpick comments (1)
backend/internal/data/repo/repo_entity_types_test.go (1)

22-41: ⚡ Quick win

Add an idempotency assertion for EnsureDefaults.

Call EnsureDefaults twice and assert the list remains exactly 2 entries. This locks the intended bootstrap contract against duplicate-default regressions.

♻️ Suggested test addition
 func TestEntityTypeRepository_EnsureDefaults(t *testing.T) {
   ctx := context.Background()
   group, err := tRepos.Groups.GroupCreate(ctx, "entity-types-defaults-"+uuid.NewString(), uuid.Nil)
   require.NoError(t, err)

   require.NoError(t, tRepos.EntityTypes.EnsureDefaults(ctx, group.ID))
+  require.NoError(t, tRepos.EntityTypes.EnsureDefaults(ctx, group.ID))

   entityTypes, err := tRepos.EntityTypes.GetAll(ctx, group.ID)
   require.NoError(t, err)
   require.Len(t, entityTypes, 2)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/internal/data/repo/repo_entity_types_test.go` around lines 22 - 41,
Add an idempotency check to TestEntityTypeRepository_EnsureDefaults: after the
first call to tRepos.EntityTypes.EnsureDefaults(ctx, group.ID) and the existing
assertions, call tRepos.EntityTypes.EnsureDefaults(ctx, group.ID) a second time,
then call tRepos.EntityTypes.GetAll(ctx, group.ID) again and assert the returned
slice still has length 2 and contains the same "Item" (IsLocation false) and
"Location" (IsLocation true) entries; this verifies EnsureDefaults is safe to
call multiple times without creating duplicate entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/internal/core/services/service_group.go`:
- Around line 37-46: The group is being persisted with
svc.repos.Groups.GroupCreate before EnsureDefaults runs, so failures in
svc.repos.EntityTypes.EnsureDefaults can leave a created group; wrap both
operations in a single atomic transaction (or, if transactions are unavailable,
delete/rollback the created group on EnsureDefaults error). Specifically, run
GroupCreate and EnsureDefaults within the same DB transaction (or call
svc.repos.Groups.Delete/Remove using group.ID on EnsureDefaults failure), using
ctx.Context and group.ID to locate the created record and ensure proper
commit/rollback so either both succeed or no group remains.

In `@backend/internal/core/services/service_user.go`:
- Around line 562-563: The log currently includes the raw OIDC subject
(log.Debug().Str("issuer", issuer).Str("subject", subject)... and the similar
log at the other location); replace the raw subject with a non-PII-safe value by
computing and logging a deterministic hash (e.g., SHA-256) or a redacted flag
and change the field name (e.g., "subject_hash" or "subject_present") so you
never emit the plain subject; update both occurrences (the Debug call that
includes subject and the other log at the second occurrence) to use the
hashed/redacted value and keep issuer logging unchanged.
- Around line 198-205: Ensure that failures from
svc.repos.EntityTypes.EnsureDefaults do not cause the API to return an error
after the user/group has already been persisted: in both places where
EnsureDefaults is called (the bootstrap paths around bootstrapSpan/span and the
second path around lines 562-568), catch the error, call recordServiceSpanError
to record it and log the failure (using log.Error or process logger), but do NOT
return repo.UserOut{} with the error; instead proceed to return the successful
UserOut response and schedule/trigger a background retry or cleanup task for
EnsureDefaults so the client sees success while the service repairs the
incomplete defaults asynchronously. Ensure you update both call sites (the calls
to svc.repos.EntityTypes.EnsureDefaults, bootstrapSpan, span, and
recordServiceSpanError) to follow this pattern.

In `@frontend/components/Location/CreateModal.vue`:
- Around line 69-70: The create payload currently drops selected tags by
hardcoding tagIds: [] — use the TagSelector-bound form.tags instead: in the
payload construction (where tagIds is set) replace the hardcoded [] with the IDs
from form.tags (e.g. tagIds: form.tags.map(t => t.id) or tagIds: form.tags if
tags are primitives). Update the submit/create handler (the function that builds
the payload for location creation in CreateModal.vue) to pull from form.tags so
user-selected or template-default tags are sent; ensure form.tags is initialized
consistently where the form is created.

In `@frontend/lib/api/__test__/user/stats.test.ts`:
- Around line 43-45: The test builds a random set of locations but HB.location
is still chosen randomly which makes totalLocations flaky; change the assignment
of HB.location to deterministically pick from the generated locations array
(locations) instead of a random choice—e.g., derive an index from a stable value
like the test iteration or suffix and assign HB.location =
locations[stableIndex] so all three generated locations are always represented
for the totalLocations assertion; update the test code that references suffix,
tags, locations and HB.location accordingly.

---

Outside diff comments:
In `@frontend/pages/template/`[id].vue:
- Around line 107-111: The mapping in template.value.fields.map currently
hardcodes the field type ("type": "text") which overwrites persisted types;
change the mapper to preserve the original type by using the existing property
(e.g., use f.type) when building each field object in the map so Template
editing does not corrupt field semantics (update the mapping in the code that
constructs fields from template.value.fields).

---

Nitpick comments:
In `@backend/internal/data/repo/repo_entity_types_test.go`:
- Around line 22-41: Add an idempotency check to
TestEntityTypeRepository_EnsureDefaults: after the first call to
tRepos.EntityTypes.EnsureDefaults(ctx, group.ID) and the existing assertions,
call tRepos.EntityTypes.EnsureDefaults(ctx, group.ID) a second time, then call
tRepos.EntityTypes.GetAll(ctx, group.ID) again and assert the returned slice
still has length 2 and contains the same "Item" (IsLocation false) and
"Location" (IsLocation true) entries; this verifies EnsureDefaults is safe to
call multiple times without creating duplicate entries.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2f5fdbb2-8a27-455d-a418-2537e67e55ea

📥 Commits

Reviewing files that changed from the base of the PR and between bf138f1 and 88edfb9.

📒 Files selected for processing (13)
  • backend/internal/core/services/main_test.go
  • backend/internal/core/services/service_group.go
  • backend/internal/core/services/service_user.go
  • backend/internal/data/repo/main_test.go
  • backend/internal/data/repo/repo_entity_types.go
  • backend/internal/data/repo/repo_entity_types_test.go
  • frontend/components/Location/CreateModal.vue
  • frontend/components/Template/Card.vue
  • frontend/components/Template/CreateModal.vue
  • frontend/lib/api/__test__/user/stats.test.ts
  • frontend/lib/template-fields.ts
  • frontend/pages/item/[id]/index.vue
  • frontend/pages/template/[id].vue
🚧 Files skipped from review as they are similar to previous changes (2)
  • frontend/components/Template/Card.vue
  • frontend/pages/item/[id]/index.vue

Comment thread backend/internal/core/services/service_group.go
Comment thread backend/internal/core/services/service_user.go
Comment thread backend/internal/core/services/service_user.go Outdated
Comment thread frontend/components/Location/CreateModal.vue Outdated
Comment thread frontend/lib/api/__test__/user/stats.test.ts
@szaiser

szaiser commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Reviews resumed.

@szaiser szaiser force-pushed the feat/paperless-immich-attachement-links branch from b7e5139 to a7a918e Compare June 30, 2026 03:03
@coderabbitai coderabbitai Bot requested a review from tankerkiller125 June 30, 2026 03:05

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/components/Template/Card.vue (1)

47-70: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve all template field value variants when duplicating.

This payload still serializes only textValue, so duplicating a template drops booleanValue, numberValue, and timeValue for non-text fields. That breaks the new template-field round-trip contract and causes silent data loss.

Proposed fix
       fields: fullTemplate.fields.map(field => ({
         id: NIL_UUID,
         name: field.name,
         type: field.type,
+        booleanValue: field.booleanValue,
+        numberValue: field.numberValue,
         textValue: field.textValue,
+        timeValue: field.timeValue,
       })),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/components/Template/Card.vue` around lines 47 - 70, The duplication
payload in the template copy logic is dropping non-text field values because the
fields mapping only carries textValue. Update the duplicate creation in Card.vue
so the data built for EntityTemplateCreate preserves all field value variants
from fullTemplate.fields, including booleanValue, numberValue, and timeValue
alongside textValue, using the existing template duplication code path and field
mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 27-41: The SSRF protection in integrationProxyHTTPClient and
proxyHTTPTransport only validates the URL before the request and during
CheckRedirect, but the transport can still resolve a different IP at connect
time. Update the V1Controller client setup to enforce validation in the actual
dial path by adding a custom DialContext on the shared http.Transport (or
pinning the vetted IP) so the final connection target is checked for both the
initial request and any redirects. Keep the existing ValidateOutboundHTTPURL
guard, but ensure the transport-level connection uses the vetted destination
instead of re-resolving the host.

In `@frontend/pages/item/`[id]/index/edit.vue:
- Around line 101-108: The location-conversion flow in the edit save path is
using the edited entity type alone, so `save()` and the redirect logic in
`edit.vue` misfire for unchanged locations and miss real location-to-item
transitions. Update the checks around `isConvertingToLocation` and the
`/location/...` redirect to compare `nullableItem.value.entityType?.isLocation`
against the edited `item.value.entityType?.isLocation`, and only treat it as a
transition when those differ. Keep the confirmation and final redirect based on
the actual before/after type change in the same save flow.

---

Outside diff comments:
In `@frontend/components/Template/Card.vue`:
- Around line 47-70: The duplication payload in the template copy logic is
dropping non-text field values because the fields mapping only carries
textValue. Update the duplicate creation in Card.vue so the data built for
EntityTemplateCreate preserves all field value variants from
fullTemplate.fields, including booleanValue, numberValue, and timeValue
alongside textValue, using the existing template duplication code path and field
mapping.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 3ad1ea1f-481f-4be4-b29c-d26adf17ccd0

📥 Commits

Reviewing files that changed from the base of the PR and between b7e5139 and a7a918e.

⛔ Files ignored due to path filters (1)
  • backend/go.sum is excluded by !**/*.sum
📒 Files selected for processing (33)
  • backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go
  • backend/app/api/handlers/v1/v1_ctrl_integration_proxy_test.go
  • backend/app/api/routes.go
  • backend/internal/core/services/main_test.go
  • backend/internal/core/services/service_group.go
  • backend/internal/core/services/service_items_attachments.go
  • backend/internal/core/services/service_items_attachments_external_test.go
  • backend/internal/core/services/service_user.go
  • backend/internal/data/repo/main_test.go
  • backend/internal/data/repo/repo_entity_types.go
  • backend/internal/data/repo/repo_entity_types_test.go
  • backend/internal/data/repo/repo_group.go
  • backend/internal/data/repo/repo_item_attachments.go
  • backend/internal/data/repo/repo_item_attachments_test.go
  • backend/internal/sys/validate/notifier_url.go
  • backend/internal/sys/validate/notifier_url_test.go
  • docs/src/content/docs/en/user-guide/notifiers.mdx
  • frontend/components/Item/AttachmentsList.vue
  • frontend/components/Template/Card.vue
  • frontend/components/Template/CreateModal.vue
  • frontend/composables/preferences-utils.ts
  • frontend/composables/use-preferences.test.ts
  • frontend/composables/use-preferences.ts
  • frontend/lib/api/__test__/user/stats.test.ts
  • frontend/lib/integration-adapters.test.ts
  • frontend/lib/integration-adapters.ts
  • frontend/lib/template-fields.ts
  • frontend/locales/en.json
  • frontend/pages/item/[id]/index.vue
  • frontend/pages/item/[id]/index/edit.vue
  • frontend/pages/profile.vue
  • frontend/pages/template/[id].vue
  • frontend/stores/integration-cache.ts
✅ Files skipped from review due to trivial changes (1)
  • frontend/lib/template-fields.ts
🚧 Files skipped from review as they are similar to previous changes (24)
  • frontend/composables/use-preferences.test.ts
  • backend/app/api/routes.go
  • frontend/lib/api/test/user/stats.test.ts
  • backend/internal/core/services/service_items_attachments.go
  • frontend/pages/template/[id].vue
  • backend/internal/data/repo/main_test.go
  • frontend/components/Template/CreateModal.vue
  • frontend/pages/item/[id]/index.vue
  • backend/internal/data/repo/repo_entity_types_test.go
  • frontend/pages/profile.vue
  • backend/internal/core/services/main_test.go
  • frontend/composables/preferences-utils.ts
  • backend/internal/data/repo/repo_group.go
  • frontend/composables/use-preferences.ts
  • frontend/locales/en.json
  • backend/internal/data/repo/repo_item_attachments_test.go
  • backend/internal/data/repo/repo_entity_types.go
  • backend/internal/data/repo/repo_item_attachments.go
  • frontend/stores/integration-cache.ts
  • frontend/lib/integration-adapters.test.ts
  • backend/internal/core/services/service_items_attachments_external_test.go
  • frontend/lib/integration-adapters.ts
  • frontend/components/Item/AttachmentsList.vue
  • backend/internal/core/services/service_user.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/components/Template/Card.vue (1)

47-70: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve all template field value variants when duplicating.

This payload still serializes only textValue, so duplicating a template drops booleanValue, numberValue, and timeValue for non-text fields. That breaks the new template-field round-trip contract and causes silent data loss.

Proposed fix
       fields: fullTemplate.fields.map(field => ({
         id: NIL_UUID,
         name: field.name,
         type: field.type,
+        booleanValue: field.booleanValue,
+        numberValue: field.numberValue,
         textValue: field.textValue,
+        timeValue: field.timeValue,
       })),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/components/Template/Card.vue` around lines 47 - 70, The duplication
payload in the template copy logic is dropping non-text field values because the
fields mapping only carries textValue. Update the duplicate creation in Card.vue
so the data built for EntityTemplateCreate preserves all field value variants
from fullTemplate.fields, including booleanValue, numberValue, and timeValue
alongside textValue, using the existing template duplication code path and field
mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 27-41: The SSRF protection in integrationProxyHTTPClient and
proxyHTTPTransport only validates the URL before the request and during
CheckRedirect, but the transport can still resolve a different IP at connect
time. Update the V1Controller client setup to enforce validation in the actual
dial path by adding a custom DialContext on the shared http.Transport (or
pinning the vetted IP) so the final connection target is checked for both the
initial request and any redirects. Keep the existing ValidateOutboundHTTPURL
guard, but ensure the transport-level connection uses the vetted destination
instead of re-resolving the host.

In `@frontend/pages/item/`[id]/index/edit.vue:
- Around line 101-108: The location-conversion flow in the edit save path is
using the edited entity type alone, so `save()` and the redirect logic in
`edit.vue` misfire for unchanged locations and miss real location-to-item
transitions. Update the checks around `isConvertingToLocation` and the
`/location/...` redirect to compare `nullableItem.value.entityType?.isLocation`
against the edited `item.value.entityType?.isLocation`, and only treat it as a
transition when those differ. Keep the confirmation and final redirect based on
the actual before/after type change in the same save flow.

---

Outside diff comments:
In `@frontend/components/Template/Card.vue`:
- Around line 47-70: The duplication payload in the template copy logic is
dropping non-text field values because the fields mapping only carries
textValue. Update the duplicate creation in Card.vue so the data built for
EntityTemplateCreate preserves all field value variants from
fullTemplate.fields, including booleanValue, numberValue, and timeValue
alongside textValue, using the existing template duplication code path and field
mapping.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 3ad1ea1f-481f-4be4-b29c-d26adf17ccd0

📥 Commits

Reviewing files that changed from the base of the PR and between b7e5139 and a7a918e.

⛔ Files ignored due to path filters (1)
  • backend/go.sum is excluded by !**/*.sum
📒 Files selected for processing (33)
  • backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go
  • backend/app/api/handlers/v1/v1_ctrl_integration_proxy_test.go
  • backend/app/api/routes.go
  • backend/internal/core/services/main_test.go
  • backend/internal/core/services/service_group.go
  • backend/internal/core/services/service_items_attachments.go
  • backend/internal/core/services/service_items_attachments_external_test.go
  • backend/internal/core/services/service_user.go
  • backend/internal/data/repo/main_test.go
  • backend/internal/data/repo/repo_entity_types.go
  • backend/internal/data/repo/repo_entity_types_test.go
  • backend/internal/data/repo/repo_group.go
  • backend/internal/data/repo/repo_item_attachments.go
  • backend/internal/data/repo/repo_item_attachments_test.go
  • backend/internal/sys/validate/notifier_url.go
  • backend/internal/sys/validate/notifier_url_test.go
  • docs/src/content/docs/en/user-guide/notifiers.mdx
  • frontend/components/Item/AttachmentsList.vue
  • frontend/components/Template/Card.vue
  • frontend/components/Template/CreateModal.vue
  • frontend/composables/preferences-utils.ts
  • frontend/composables/use-preferences.test.ts
  • frontend/composables/use-preferences.ts
  • frontend/lib/api/__test__/user/stats.test.ts
  • frontend/lib/integration-adapters.test.ts
  • frontend/lib/integration-adapters.ts
  • frontend/lib/template-fields.ts
  • frontend/locales/en.json
  • frontend/pages/item/[id]/index.vue
  • frontend/pages/item/[id]/index/edit.vue
  • frontend/pages/profile.vue
  • frontend/pages/template/[id].vue
  • frontend/stores/integration-cache.ts
✅ Files skipped from review due to trivial changes (1)
  • frontend/lib/template-fields.ts
🚧 Files skipped from review as they are similar to previous changes (24)
  • frontend/composables/use-preferences.test.ts
  • backend/app/api/routes.go
  • frontend/lib/api/test/user/stats.test.ts
  • backend/internal/core/services/service_items_attachments.go
  • frontend/pages/template/[id].vue
  • backend/internal/data/repo/main_test.go
  • frontend/components/Template/CreateModal.vue
  • frontend/pages/item/[id]/index.vue
  • backend/internal/data/repo/repo_entity_types_test.go
  • frontend/pages/profile.vue
  • backend/internal/core/services/main_test.go
  • frontend/composables/preferences-utils.ts
  • backend/internal/data/repo/repo_group.go
  • frontend/composables/use-preferences.ts
  • frontend/locales/en.json
  • backend/internal/data/repo/repo_item_attachments_test.go
  • backend/internal/data/repo/repo_entity_types.go
  • backend/internal/data/repo/repo_item_attachments.go
  • frontend/stores/integration-cache.ts
  • frontend/lib/integration-adapters.test.ts
  • backend/internal/core/services/service_items_attachments_external_test.go
  • frontend/lib/integration-adapters.ts
  • frontend/components/Item/AttachmentsList.vue
  • backend/internal/core/services/service_user.go
🛑 Comments failed to post (2)
backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go (1)

27-41: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether the integration proxy transport pins validation to the actual dial target.
rg -n -C3 'proxyHTTPTransport|DialContext|CheckRedirect|ValidateOutboundHTTPURL|\.Do\(req\)' \
  backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go \
  backend/internal/sys/validate/notifier_url.go

Repository: sysadminsmedia/homebox

Length of output: 4351


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the relevant files before reading the key sections.
ast-grep outline backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go --view expanded
printf '\n---\n'
ast-grep outline backend/internal/sys/validate/notifier_url.go --view expanded
printf '\n---\n'
wc -l backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go backend/internal/sys/validate/notifier_url.go

Repository: sysadminsmedia/homebox

Length of output: 1534


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '1,220p' backend/internal/sys/validate/notifier_url.go
printf '\n---\n'
sed -n '1,170p' backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go

Repository: sysadminsmedia/homebox

Length of output: 13397


Enforce SSRF checks at dial time. Preflight validation and CheckRedirect still leave a DNS-rebinding window because http.Transport resolves the host again when it connects. Use a custom DialContext or pin the vetted IP so the actual connection target is checked for both the initial request and redirects.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go` around lines 27 -
41, The SSRF protection in integrationProxyHTTPClient and proxyHTTPTransport
only validates the URL before the request and during CheckRedirect, but the
transport can still resolve a different IP at connect time. Update the
V1Controller client setup to enforce validation in the actual dial path by
adding a custom DialContext on the shared http.Transport (or pinning the vetted
IP) so the final connection target is checked for both the initial request and
any redirects. Keep the existing ValidateOutboundHTTPURL guard, but ensure the
transport-level connection uses the vetted destination instead of re-resolving
the host.
frontend/pages/item/[id]/index/edit.vue (1)

101-108: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Track the actual location-type transition, not just the new type.

Right now this flag is true for any item whose edited entityType is a location, so existing locations get the confirmation and /location/... redirect on every save, while real location → item changes skip the confirmation entirely. Compare the original nullableItem.value.entityType?.isLocation with the edited value, then redirect from the final type only when the type actually changed.

Suggested fix
-    const isConvertingToLocation = item.value.entityType?.isLocation;
-    if (isConvertingToLocation) {
+    const wasLocation = nullableItem.value?.entityType?.isLocation ?? false;
+    const isLocation = item.value.entityType?.isLocation ?? false;
+    const changedLocationKind = wasLocation !== isLocation;
+    if (changedLocationKind) {
       const { isCanceled } = await confirm.open(t("items.edit.change_entity_type_confirm"));
       if (isCanceled) {
         saving.value = false;
         return;
       }
@@
-    if (isConvertingToLocation) {
-      navigateTo("/location/" + itemId.value);
-    } else if (redirect) {
-      navigateTo("/item/" + itemId.value);
+    if (changedLocationKind || redirect) {
+      navigateTo((isLocation ? "/location/" : "/item/") + itemId.value);
     }

Also applies to: 146-149

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/pages/item/`[id]/index/edit.vue around lines 101 - 108, The
location-conversion flow in the edit save path is using the edited entity type
alone, so `save()` and the redirect logic in `edit.vue` misfire for unchanged
locations and miss real location-to-item transitions. Update the checks around
`isConvertingToLocation` and the `/location/...` redirect to compare
`nullableItem.value.entityType?.isLocation` against the edited
`item.value.entityType?.isLocation`, and only treat it as a transition when
those differ. Keep the confirmation and final redirect based on the actual
before/after type change in the same save flow.

@szaiser

szaiser commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai resume

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/components/Item/View/table/columns.ts (1)

156-163: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Guard item.parent with entityType?.isLocation before rendering the link. parent can be a non-location entity, so /location/${loc.id} can point to the wrong page when the item is nested under another item.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/components/Item/View/table/columns.ts` around lines 156 - 163, The
link rendering in the table cell currently assumes item.parent is always a
location, which can send users to the wrong page. Update the cell renderer in
columns.ts to only render the /location/${loc.id} anchor when
entityType?.isLocation is true and item.parent exists; otherwise fall back to
the muted placeholder. Use the existing row.original EntitySummary, item.parent,
and entityType check in this column definition to keep the behavior scoped to
actual locations.
🧹 Nitpick comments (1)
backend/internal/sys/validate/notifier_url.go (1)

62-73: 🔒 Security & Privacy | 🔵 Trivial

Verify the proxy’s default SSRF policy for user-configured integrations.

This transport enforces the supplied NotifierConf; the provided config defaults leave localhost/private blocking off. For user-controlled integration URLs, either document the required hardening flags or use a stricter proxy-specific policy if LAN targets are not intentionally allowed.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/internal/sys/validate/notifier_url.go` around lines 62 - 73, The
NewOutboundHTTPTransport path is using the raw NotifierConf defaults, which
leave localhost/private-network blocking disabled for user-configured
integrations. Update the notifier URL handling to either apply a stricter
proxy-specific outbound policy by default for
outboundDialContext/NewOutboundHTTPTransport, or clearly require and document
the hardening flags that callers must enable when using user-controlled URLs.
Ensure the final policy in notifier_url.go is explicit about whether LAN/private
targets are allowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/internal/sys/validate/notifier_url.go`:
- Around line 54-57: The DNS validation in validateNotifierURL currently uses
context.Background(), which can outlive request timeouts; update the call in
validateNotifierURL to use a bounded context instead. Prefer threading the
caller’s context through the validation path and into resolveOutboundHost, or
otherwise create a deadline-limited context before resolving the host so
outbound DNS lookups respect proxy/request limits.
- Around line 93-106: The network policy check in `validateNotifierURL` is
running before the dial-network filter, so unreachable IP families can fail
validation even when a valid address exists. Update the `validateOutboundIPs`
flow to only consider IPs that match the requested network (`tcp4`/`tcp6`)
before applying block/allow policy, using the existing `ipMatchesNetwork` logic
in `validate/notifier_url.go`. Keep the `resolveOutboundHost`,
`validateOutboundIPs`, and `dialer.DialContext` path intact, but ensure
validation is scoped to the filtered IP list instead of all resolved addresses.

In `@frontend/components/Item/View/table/data-table-expanded-row.vue`:
- Line 15: The expanded row and columns logic is using props.item.parent as if
it were always a location, which can produce invalid /location links for
non-location parents. Update the computed location in
data-table-expanded-row.vue and the related parent-link handling in columns.ts
to only treat parent as a location when parent?.entityType?.isLocation is true,
otherwise leave it unset so the link is not rendered.

In `@frontend/pages/reports/label-generator.vue`:
- Around line 228-241: The location mapping in getItem is too permissive because
item?.parent?.name can populate the label with any parent name instead of only
locations. Update getItem in label-generator.vue to use the same parent entity
type guard as saveAsTemplate, checking item.parent?.entityType?.isLocation
before assigning location, and fall back to labelBlankLine otherwise. Keep the
existing assetID/name behavior unchanged.

---

Outside diff comments:
In `@frontend/components/Item/View/table/columns.ts`:
- Around line 156-163: The link rendering in the table cell currently assumes
item.parent is always a location, which can send users to the wrong page. Update
the cell renderer in columns.ts to only render the /location/${loc.id} anchor
when entityType?.isLocation is true and item.parent exists; otherwise fall back
to the muted placeholder. Use the existing row.original EntitySummary,
item.parent, and entityType check in this column definition to keep the behavior
scoped to actual locations.

---

Nitpick comments:
In `@backend/internal/sys/validate/notifier_url.go`:
- Around line 62-73: The NewOutboundHTTPTransport path is using the raw
NotifierConf defaults, which leave localhost/private-network blocking disabled
for user-configured integrations. Update the notifier URL handling to either
apply a stricter proxy-specific outbound policy by default for
outboundDialContext/NewOutboundHTTPTransport, or clearly require and document
the hardening flags that callers must enable when using user-controlled URLs.
Ensure the final policy in notifier_url.go is explicit about whether LAN/private
targets are allowed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 3b3ee73b-1cab-4468-9fe0-a9813dd6cb9d

📥 Commits

Reviewing files that changed from the base of the PR and between a7a918e and ccec3d8.

📒 Files selected for processing (28)
  • backend/app/api/handlers/v1/controller.go
  • backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go
  • backend/internal/core/services/service_items_attachments_external_test.go
  • backend/internal/sys/config/conf.go
  • backend/internal/sys/validate/notifier_url.go
  • backend/internal/sys/validate/notifier_url_test.go
  • frontend/components/Entity/CreateModal.vue
  • frontend/components/Item/AttachmentsList.vue
  • frontend/components/Item/View/ItemChangeDetails.vue
  • frontend/components/Item/View/table/columns.ts
  • frontend/components/Item/View/table/data-table-dropdown.vue
  • frontend/components/Item/View/table/data-table-expanded-row.vue
  • frontend/components/Scanner/AROverlayCard.vue
  • frontend/components/Template/Card.vue
  • frontend/composables/use-barcode-detector.ts
  • frontend/lib/api/__test__/factories/index.ts
  • frontend/lib/api/__test__/user/items.test.ts
  • frontend/lib/api/__test__/user/tags.test.ts
  • frontend/lib/api/__test__/user/templates.test.ts
  • frontend/lib/api/classes/items.ts
  • frontend/lib/integration-adapters.ts
  • frontend/pages/collection/index/tools.vue
  • frontend/pages/item/[id]/index.vue
  • frontend/pages/item/[id]/index/edit.vue
  • frontend/pages/profile.vue
  • frontend/pages/reports/label-generator.vue
  • frontend/pages/template/[id].vue
  • frontend/stores/integration-cache.ts
✅ Files skipped from review due to trivial changes (2)
  • backend/internal/sys/config/conf.go
  • frontend/lib/api/test/user/tags.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
  • backend/app/api/handlers/v1/controller.go
  • frontend/components/Template/Card.vue
  • frontend/pages/template/[id].vue
  • frontend/pages/item/[id]/index/edit.vue
  • frontend/pages/profile.vue
  • frontend/stores/integration-cache.ts
  • backend/internal/core/services/service_items_attachments_external_test.go
  • backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go
  • frontend/lib/integration-adapters.ts
  • backend/internal/sys/validate/notifier_url_test.go
  • frontend/components/Item/AttachmentsList.vue

Comment thread backend/internal/sys/validate/notifier_url.go Outdated
Comment thread backend/internal/sys/validate/notifier_url.go
Comment thread frontend/components/Item/View/table/data-table-expanded-row.vue Outdated
Comment thread frontend/pages/reports/label-generator.vue Outdated
@szaiser

szaiser commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@tankerkiller125 With the latest updates, Homebox's shared outbound URL validation and transport are now used for the integration proxy as requested, replacing the feature-specific SSRF handling. Additionally, CodeRabbit-requested cleanup has been performed, including bounded DNS validation, network-family filtering before policy checks, and parent-as-location guards.

Please let me know what other fixes/ architectural changes need to be done.

@szaiser

szaiser commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
frontend/pages/item/[id]/index.vue (1)

601-608: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve the source field type when saving templates.

These lines now copy the variant values, but Line 604 still forces every saved field to type: "text". A boolean/number/time custom field saved as a template will therefore round-trip with the wrong schema.

Suggested fix
       fields: item.value.fields.map(field => ({
         id: NIL_UUID,
         name: field.name,
-        type: "text",
+        type: field.type,
         booleanValue: field.booleanValue,
         numberValue: field.numberValue,
         textValue: field.textValue || "",
         timeValue: DEFAULT_TEMPLATE_FIELD_TIME_VALUE,
       })),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/pages/item/`[id]/index.vue around lines 601 - 608, The template-save
mapping is overriding every field to text instead of preserving each source
field’s schema. In the item template creation logic where item.value.fields is
mapped, set the saved field type from the original field’s type rather than
hardcoding text, while still copying the matching variant value properties so
boolean, number, and time fields round-trip correctly.
frontend/components/Item/View/table/data-table-dropdown.vue (1)

63-72: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Location export is now blank for every row.

With EntitySummary, item.original["location"] no longer exists, so the CSV path exports an empty field even though the table renders location from parent. The same raw-key lookup in downloadJson() below has the same regression. Special-case derived columns like location when building export values.

Suggested fix
+  const getExportValue = (item: EntitySummary, col: string) => {
+    if (col === "location") {
+      return item.parent?.entityType?.isLocation ? item.parent.name : "";
+    }
+
+    return item[col as keyof EntitySummary] ?? "";
+  };
+
   const downloadCsv = (items: Row<EntitySummary>[], columns: Column<EntitySummary>[]) => {
@@
     const rows = items.map(item =>
-      enabledColumns.map(col => formatValueAsCsvField(item.original[col as keyof EntitySummary])).join(",")
+      enabledColumns.map(col => formatValueAsCsvField(getExportValue(item.original, col))).join(",")
     );
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/components/Item/View/table/data-table-dropdown.vue` around lines 63
- 72, The export logic in downloadCsv and downloadJson is using raw
EntitySummary keys, so derived table columns like location end up blank because
item.original["location"] does not exist. Update the row-building logic in these
helpers to special-case location and pull the displayed value from parent (the
same source the table uses), while keeping the existing key-based lookup for
normal fields. Make sure both CSV and JSON export paths use the same
derived-value handling so the exported data matches what the table renders.
♻️ Duplicate comments (2)
backend/internal/data/repo/repo_entity_types.go (1)

90-111: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Make ensureDefaultEntityType concurrency-safe.

This is still a check-then-create race. A foreground EnsureDefaults call and the new background retry path in backend/internal/core/services/service_user.go can both miss the same row, then either insert duplicates or surface a constraint error. This helper needs an atomic upsert / constraint-retry path here, plus a unique (group_id, is_location) guarantee if that is not already enforced.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/internal/data/repo/repo_entity_types.go` around lines 90 - 111,
`ensureDefaultEntityType` still does a check-then-create flow, so concurrent
callers can both miss the same `EntityType` and race on insert. Update the
`entityTypes.Query().First` / `entityTypes.Create` path to use an atomic upsert
or a retry-on-unique-constraint pattern inside `ensureDefaultEntityType`, and
make sure the `EntityType` model enforces a unique `(group_id, is_location)`
constraint if it does not already. Keep the return contract the same, but ensure
the create path is safe when both `EnsureDefaults` and the retry logic in
`service_user.go` run at once.
backend/internal/core/services/service_user.go (1)

219-226: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

The retryable bootstrap still fails the request later in the same flow.

Line 220 and Line 598 downgrade EnsureDefaults failures to log+retry, but Line 257 and Line 635 still call ensureDefaultEntityTypes(...) and return the error after the user/group has already been persisted. That reintroduces the partial-success contract break this change is trying to remove. Drop the later hard-fail path or make it follow the same best-effort retry behavior.

Also applies to: 597-606

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/internal/core/services/service_user.go` around lines 219 - 226, The
`EnsureDefaults` bootstrap path is being treated as retryable in
`service_user.go`, but the later `ensureDefaultEntityTypes(...)` call still
returns a hard error after the user/group has already been persisted. Update the
`register_user` flow so the later `ensureDefaultEntityTypes` branch follows the
same best-effort behavior as the earlier `svc.repos.EntityTypes.EnsureDefaults`
handling: log, record the span error, schedule `retryEntityTypeDefaults`, and do
not propagate the failure as a request error. Keep the behavior consistent
across both call sites that currently enforce the default entity type setup.
🧹 Nitpick comments (2)
backend/internal/data/repo/repo_item_attachments_test.go (1)

229-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a direct repo test for the unsupported-MIME branch.

The new !isExternalLink(mimeType) guard in CreateExternalLink is only covered indirectly today. A dedicated repo test would lock in that contract for any future caller that bypasses MimeTypeForSourceType.

🧪 Proposed test
+func TestAttachmentRepo_CreateExternalLink_InvalidMimeType(t *testing.T) {
+	ctx := context.Background()
+	entity := useEntities(t, 1)[0]
+
+	_, err := tRepos.Attachments.CreateExternalLink(
+		ctx,
+		entity.ID,
+		"42",
+		"Bad MIME",
+		"application/pdf",
+		attachment.TypeAttachment,
+	)
+	assert.Error(t, err)
+}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/internal/data/repo/repo_item_attachments_test.go` around lines 229 -
235, Add a direct repository test for the unsupported-MIME guard in
CreateExternalLink, since the new !isExternalLink(mimeType) path is only
exercised indirectly today. Extend the repo test coverage in
TestAttachmentRepo_CreateExternalLink_InvalidEntityID or add a sibling test that
calls tRepos.Attachments.CreateExternalLink with a valid entity ID but a MIME
type that is not an external link, and assert it returns an error. Use
CreateExternalLink and the existing attachment test helpers/cases to keep the
test focused on this branch.
frontend/composables/use-preferences.ts (1)

2-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Collapse the duplicate preferences-utils import/re-export.

This import plus export ... from "./preferences-utils" pattern is what the frontend pipeline is warning about. Re-export the local bindings instead so this file stops generating duplicate-import noise.

Minimal cleanup
 import {
   type LocationViewPreferences,
   type PreferenceSyncConfig,
   DEFAULT_PREFERENCES,
   buildSyncedSettings,
   mergeSyncedSettings,
 } from "./preferences-utils";
 
-export type { ViewType, DuplicateSettings, LocationViewPreferences, PreferenceSyncConfig } from "./preferences-utils";
-export { DEFAULT_PREFERENCES, buildSyncedSettings, mergeSyncedSettings } from "./preferences-utils";
+export type { ViewType, DuplicateSettings, LocationViewPreferences, PreferenceSyncConfig };
+export { DEFAULT_PREFERENCES, buildSyncedSettings, mergeSyncedSettings };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/composables/use-preferences.ts` around lines 2 - 11, The
use-preferences module is importing from preferences-utils and then re-exporting
from the same module, which triggers duplicate-import noise in the frontend
pipeline. Update the use-preferences file to re-export the already imported
bindings directly, using the local symbols from the import statement instead of
a separate export-from clause; keep the existing symbols such as
DEFAULT_PREFERENCES, buildSyncedSettings, mergeSyncedSettings,
LocationViewPreferences, and PreferenceSyncConfig wired through the same module.

Source: Pipeline failures

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 65-124: The proxy handler in HandleUserSelfSettingsUpdate only
validates the URL-safe format of name, which still allows arbitrary integration
keys to be used for authenticated outbound requests. Add an explicit allowlist
check for supported integration names before reading {name}_url and
{name}_token, and return a bad request error for anything unsupported. Keep the
existing name validation, but gate the proxy path earlier in the handler that
uses validIntegrationName, settings, and
validate.ValidateOutboundHTTPURLWithContext.

In `@backend/internal/core/services/service_items_attachments.go`:
- Around line 21-36: The trace redaction in redactExternalIdentifierForTrace
still exposes the raw host/port for "link" sources, so update it to redact or
hash the hostname as well before returning the value used for
integration.external_id spans. Keep the existing path hashing logic, but replace
any direct use of u.Host in the formatted trace identifier with a sanitized host
representation so no user-supplied hostname leaks through. Also make sure the
call site that records integration.external_id uses the updated redaction
output.

In `@frontend/components/Entity/CreateModal.vue`:
- Around line 623-634: The template creation path is still using only
form.location?.id for parentId, which ignores the selected parent item and can
place templated sub-items at the wrong level. Update the parentId resolution in
CreateModal.vue so the template branch uses the same fallback logic as the
non-template branch (prefer form.parentId, then location as fallback), and apply
the same fix in the template request construction around the template creation
flow.

In `@frontend/components/Item/AttachmentsList.vue`:
- Around line 72-85: The AttachmentsList.vue template still contains hardcoded
English literals, so move the remaining `pages` text and the `aria-label` on the
`MdiAlertCircleOutline` inside the item actions behind existing i18n usage.
Update the relevant template expressions in `AttachmentsList.vue` to pull
translated strings through the same translation mechanism used elsewhere in the
component, keeping the `paperlessDoc(attachment)` and
`isUnreachable(attachment)` logic unchanged.
- Around line 113-140: The external-link rendering branch in AttachmentsList.vue
is binding attachment.path directly into anchor hrefs without validating the
scheme. Update the isLink(attachment) path in AttachmentsList to reuse the
existing HTTP(S) URL check used elsewhere in the item attachment flow, and only
render the two anchors when the URL is safe. If attachment.path is not a valid
HTTP(S) URL, fall back to plain text rendering instead of a clickable link.

In `@frontend/components/Item/View/ItemChangeDetails.vue`:
- Around line 61-65: Reset newLocation before computing the default selection in
ItemChangeDetails.vue so reopening the dialog cannot reuse a stale parent from a
previous run. In the dialog setup/watch logic where params.changeLocation and
params.items are evaluated, clear newLocation first and only assign firstParent
when all selected items share the same location parent; otherwise leave it unset
so save() cannot submit an old parentId.

In `@frontend/composables/preferences-utils.ts`:
- Around line 65-100: The synced preference key list is incorrectly derived from
DEFAULT_PREFERENCES, so optional LocationViewPreferences fields like
tableHeaders and collectionId are skipped during serialization and merge. Update
preferences-utils.ts to source the syncable keys from the full
LocationViewPreferences shape or an explicit shared key list instead of
DEFAULT_PREFERENCES, and make forEachSyncedPreference, buildSyncedSettings, and
mergeSyncedSettings operate over that complete set so enabled optional
preferences are preserved.

In `@frontend/composables/use-preferences.ts`:
- Around line 88-90: Abort the save flow in use-preferences when
api.user.getSettings() fails or returns no item, instead of building merged from
an empty object. Update the logic around buildSyncedSettings and
api.user.setSettings so the write only proceeds after a successful read with
current.item present; otherwise return early and preserve existing settings,
including unrelated integrations and tokens.

In `@frontend/lib/integration-adapters.ts`:
- Around line 50-65: In integration-adapters.ts, the helper that computes the
extracted ID currently falls back to pattern-only matching even when a
configured baseUrl is present but invalid, which can incorrectly accept links;
update the logic around the URL parsing block and the path extraction in this
function so that any non-empty baseUrl that fails to parse returns null
immediately, while preserving the existing heuristic pattern-only fallback only
when baseUrl is undefined. Use the existing URL matching flow in this helper
(including target.origin, basePath, and the final pathAfterBase.match call) to
keep the valid configured-base behavior unchanged.

In `@frontend/pages/reports/label-generator.vue`:
- Around line 268-271: The top-level item path in label generation is being
altered by passing null into getItem, which clears the asset ID and name for
root items. In label-generator.vue, update the branch around getItem so that
items without item.parent are still passed through as the original item, letting
getItem() handle the missing-location case itself; keep the behavior for
parented items unchanged.

In `@frontend/stores/integration-cache.ts`:
- Around line 110-128: The enrichment cache key is too broad because
getEnrichedData, setEnrichedData, and invalidateEnrichedData only namespace by
serviceName and id, so persisted localStorage entries can leak across users or
different Paperless URLs. Update the cache keying in integration-cache.ts to
include the authenticated user id and a normalized service base URL, or
alternatively clear all enrichment entries when setServiceUrl() changes to a new
backend. Make sure the same namespacing/clear behavior is applied consistently
in the read, write, and invalidate paths so stale metadata cannot be reused
across accounts or service instances.

---

Outside diff comments:
In `@frontend/components/Item/View/table/data-table-dropdown.vue`:
- Around line 63-72: The export logic in downloadCsv and downloadJson is using
raw EntitySummary keys, so derived table columns like location end up blank
because item.original["location"] does not exist. Update the row-building logic
in these helpers to special-case location and pull the displayed value from
parent (the same source the table uses), while keeping the existing key-based
lookup for normal fields. Make sure both CSV and JSON export paths use the same
derived-value handling so the exported data matches what the table renders.

In `@frontend/pages/item/`[id]/index.vue:
- Around line 601-608: The template-save mapping is overriding every field to
text instead of preserving each source field’s schema. In the item template
creation logic where item.value.fields is mapped, set the saved field type from
the original field’s type rather than hardcoding text, while still copying the
matching variant value properties so boolean, number, and time fields round-trip
correctly.

---

Duplicate comments:
In `@backend/internal/core/services/service_user.go`:
- Around line 219-226: The `EnsureDefaults` bootstrap path is being treated as
retryable in `service_user.go`, but the later `ensureDefaultEntityTypes(...)`
call still returns a hard error after the user/group has already been persisted.
Update the `register_user` flow so the later `ensureDefaultEntityTypes` branch
follows the same best-effort behavior as the earlier
`svc.repos.EntityTypes.EnsureDefaults` handling: log, record the span error,
schedule `retryEntityTypeDefaults`, and do not propagate the failure as a
request error. Keep the behavior consistent across both call sites that
currently enforce the default entity type setup.

In `@backend/internal/data/repo/repo_entity_types.go`:
- Around line 90-111: `ensureDefaultEntityType` still does a check-then-create
flow, so concurrent callers can both miss the same `EntityType` and race on
insert. Update the `entityTypes.Query().First` / `entityTypes.Create` path to
use an atomic upsert or a retry-on-unique-constraint pattern inside
`ensureDefaultEntityType`, and make sure the `EntityType` model enforces a
unique `(group_id, is_location)` constraint if it does not already. Keep the
return contract the same, but ensure the create path is safe when both
`EnsureDefaults` and the retry logic in `service_user.go` run at once.

---

Nitpick comments:
In `@backend/internal/data/repo/repo_item_attachments_test.go`:
- Around line 229-235: Add a direct repository test for the unsupported-MIME
guard in CreateExternalLink, since the new !isExternalLink(mimeType) path is
only exercised indirectly today. Extend the repo test coverage in
TestAttachmentRepo_CreateExternalLink_InvalidEntityID or add a sibling test that
calls tRepos.Attachments.CreateExternalLink with a valid entity ID but a MIME
type that is not an external link, and assert it returns an error. Use
CreateExternalLink and the existing attachment test helpers/cases to keep the
test focused on this branch.

In `@frontend/composables/use-preferences.ts`:
- Around line 2-11: The use-preferences module is importing from
preferences-utils and then re-exporting from the same module, which triggers
duplicate-import noise in the frontend pipeline. Update the use-preferences file
to re-export the already imported bindings directly, using the local symbols
from the import statement instead of a separate export-from clause; keep the
existing symbols such as DEFAULT_PREFERENCES, buildSyncedSettings,
mergeSyncedSettings, LocationViewPreferences, and PreferenceSyncConfig wired
through the same module.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ec5c70ad-e4b3-49c5-b0c8-71fef7b00e49

📥 Commits

Reviewing files that changed from the base of the PR and between 0e97012 and 51a4f99.

⛔ Files ignored due to path filters (1)
  • backend/go.sum is excluded by !**/*.sum
📒 Files selected for processing (49)
  • backend/app/api/handlers/v1/controller.go
  • backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go
  • backend/app/api/handlers/v1/v1_ctrl_integration_proxy_test.go
  • backend/app/api/routes.go
  • backend/internal/core/services/main_test.go
  • backend/internal/core/services/service_group.go
  • backend/internal/core/services/service_items_attachments.go
  • backend/internal/core/services/service_items_attachments_external_test.go
  • backend/internal/core/services/service_user.go
  • backend/internal/data/repo/main_test.go
  • backend/internal/data/repo/repo_entity_types.go
  • backend/internal/data/repo/repo_entity_types_test.go
  • backend/internal/data/repo/repo_group.go
  • backend/internal/data/repo/repo_item_attachments.go
  • backend/internal/data/repo/repo_item_attachments_test.go
  • backend/internal/sys/config/conf.go
  • backend/internal/sys/validate/notifier_url.go
  • backend/internal/sys/validate/notifier_url_test.go
  • docs/src/content/docs/en/user-guide/notifiers.mdx
  • frontend/components/Entity/CreateModal.vue
  • frontend/components/Item/AttachmentsList.vue
  • frontend/components/Item/View/ItemChangeDetails.vue
  • frontend/components/Item/View/table/columns.ts
  • frontend/components/Item/View/table/data-table-dropdown.vue
  • frontend/components/Item/View/table/data-table-expanded-row.vue
  • frontend/components/Scanner/AROverlayCard.vue
  • frontend/components/Template/Card.vue
  • frontend/components/Template/CreateModal.vue
  • frontend/composables/preferences-utils.ts
  • frontend/composables/use-barcode-detector.ts
  • frontend/composables/use-preferences.test.ts
  • frontend/composables/use-preferences.ts
  • frontend/lib/api/__test__/factories/index.ts
  • frontend/lib/api/__test__/user/items.test.ts
  • frontend/lib/api/__test__/user/stats.test.ts
  • frontend/lib/api/__test__/user/tags.test.ts
  • frontend/lib/api/__test__/user/templates.test.ts
  • frontend/lib/api/classes/items.ts
  • frontend/lib/integration-adapters.test.ts
  • frontend/lib/integration-adapters.ts
  • frontend/lib/template-fields.ts
  • frontend/locales/en.json
  • frontend/pages/collection/index/tools.vue
  • frontend/pages/item/[id]/index.vue
  • frontend/pages/item/[id]/index/edit.vue
  • frontend/pages/profile.vue
  • frontend/pages/reports/label-generator.vue
  • frontend/pages/template/[id].vue
  • frontend/stores/integration-cache.ts

Comment on lines +65 to +124
name := chi.URLParam(r, "name")
if !validIntegrationName.MatchString(name) {
return validate.NewRequestError(fmt.Errorf("invalid integration name"), http.StatusBadRequest)
}

rawPath := r.URL.Query().Get("path")
if rawPath == "" {
return validate.NewRequestError(fmt.Errorf("path query parameter is required"), http.StatusBadRequest)
}
if !strings.HasPrefix(rawPath, "/") || strings.Contains(rawPath, "://") {
return validate.NewRequestError(fmt.Errorf("path must be a relative path starting with /"), http.StatusBadRequest)
}

// Normalise to prevent directory traversal while preserving trailing slash
// (many REST APIs treat /foo/1/ and /foo/1 differently).
cleanPath := path.Clean(rawPath)
if !strings.HasPrefix(cleanPath, "/") {
return validate.NewRequestError(fmt.Errorf("invalid path after normalisation"), http.StatusBadRequest)
}
if strings.HasSuffix(rawPath, "/") && !strings.HasSuffix(cleanPath, "/") {
cleanPath += "/"
}

span.SetAttributes(
attribute.String("integration.name", name),
attribute.String("integration.path", cleanPath),
)

ctx := services.NewContext(spanCtx)
settings, svcErr := ctrl.svc.User.GetSettings(ctx.Context, services.UseUserCtx(ctx.Context).ID)
if svcErr != nil {
return validate.NewRequestError(svcErr, http.StatusInternalServerError)
}

baseURL, _ := settings[name+"_url"].(string)
if baseURL == "" {
return validate.NewRequestError(
fmt.Errorf("%s_url not configured – add it in Settings", name),
http.StatusBadRequest,
)
}

upstream := strings.TrimRight(baseURL, "/") + cleanPath
if err := validate.ValidateOutboundHTTPURLWithContext(r.Context(), upstream, &ctrl.config.Notifier); err != nil {
return validate.NewRequestError(err, http.StatusBadRequest)
}

token, _ := settings[name+"_token"].(string)
if token == "" {
return validate.NewRequestError(
fmt.Errorf("%s_token not configured – add it in Settings", name),
http.StatusBadRequest,
)
}

req, err := http.NewRequestWithContext(r.Context(), http.MethodGet, upstream, nil)
if err != nil {
return validate.NewRequestError(err, http.StatusBadRequest)
}
req.Header.Set("Authorization", "Token "+token)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
ast-grep outline backend/app/api/handlers/v1 --match HandleUserSelfSettingsUpdate --view expanded

rg -n -C3 'HandleUserSelfSettingsUpdate|GetSettings|SetSettings|UpdateSettings|validIntegrationName|"_url"|"_token"' \
  backend/app/api/handlers/v1 backend/internal

Repository: sysadminsmedia/homebox

Length of output: 13671


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the user settings update path and any server-side validation/allowlist for settings keys.
sed -n '205,260p' backend/app/api/handlers/v1/v1_ctrl_user.go
printf '\n---\n'
sed -n '1,220p' backend/internal/core/services/service_user.go | sed -n '790,840p'
printf '\n---\n'
sed -n '340,390p' backend/internal/data/repo/repo_users.go
printf '\n---\n'
rg -n -C2 'paperless|integrat|_url"|_token"|settings.*allow|allowlist|whitelist|validation.*settings|map\[string\]interface' backend/app backend/internal

Repository: sysadminsmedia/homebox

Length of output: 40809


🏁 Script executed:

#!/bin/bash
python3 - <<'PY'
from pathlib import Path
p = Path('backend/app/api/handlers/v1/v1_ctrl_user.go')
print(p.exists())
PY

Repository: sysadminsmedia/homebox

Length of output: 165


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '205,260p' backend/app/api/handlers/v1/v1_ctrl_user.go
printf '\n---\n'
sed -n '760,815p' backend/internal/core/services/service_user.go
printf '\n---\n'
sed -n '363,380p' backend/internal/data/repo/repo_users.go
printf '\n---\n'
rg -n -C2 'validIntegrationName|name\\+\"_url\"|name\\+\"_token\"|SetSettings\\(|GetSettings\\(' backend/app/api/handlers/v1 backend/internal

Repository: sysadminsmedia/homebox

Length of output: 5797


🏁 Script executed:

#!/bin/bash
python3 - <<'PY'
from pathlib import Path
p = Path('backend/app/api/handlers/v1/v1_ctrl_user.go')
print(p.read_text().splitlines()[204:225])
PY

Repository: sysadminsmedia/homebox

Length of output: 1350


Gate name to supported integrations. HandleUserSelfSettingsUpdate persists arbitrary settings keys, so regex-only validation still allows any lower-case {name} to become an authenticated outbound proxy via {name}_url / {name}_token.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go` around lines 65 -
124, The proxy handler in HandleUserSelfSettingsUpdate only validates the
URL-safe format of name, which still allows arbitrary integration keys to be
used for authenticated outbound requests. Add an explicit allowlist check for
supported integration names before reading {name}_url and {name}_token, and
return a bad request error for anything unsupported. Keep the existing name
validation, but gate the proxy path earlier in the handler that uses
validIntegrationName, settings, and validate.ValidateOutboundHTTPURLWithContext.

Comment thread backend/internal/core/services/service_items_attachments.go
Comment thread frontend/components/Entity/CreateModal.vue Outdated
Comment thread frontend/components/Item/AttachmentsList.vue Outdated
Comment thread frontend/components/Item/AttachmentsList.vue Outdated
Comment on lines +65 to +100
const preferenceKeys = Object.keys(DEFAULT_PREFERENCES) as (keyof LocationViewPreferences)[];

export function forEachSyncedPreference(
syncConfig: PreferenceSyncConfig,
callback: (key: keyof LocationViewPreferences) => void
) {
for (const key of preferenceKeys) {
if (syncConfig[key] !== false) {
callback(key);
}
}
}

export function buildSyncedSettings(
preferences: LocationViewPreferences,
syncConfig: PreferenceSyncConfig
): Record<string, unknown> {
const payload: Record<string, unknown> = {};
forEachSyncedPreference(syncConfig, key => {
payload[key] = preferences[key];
});
return payload;
}

export function mergeSyncedSettings(
settings: Record<string, unknown>,
preferences: LocationViewPreferences,
syncConfig: PreferenceSyncConfig
): LocationViewPreferences {
const nextPreferences = { ...preferences };
forEachSyncedPreference(syncConfig, key => {
if (key in settings) {
nextPreferences[key] = settings[key] as never;
}
});
return nextPreferences;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Don't derive syncable keys from the default object.

preferenceKeys only includes keys present in DEFAULT_PREFERENCES, so optional LocationViewPreferences members like tableHeaders and collectionId can never be serialized or merged even when they exist and syncConfig enables them. That makes the exported sync contract lie and silently drops valid settings.

Suggested direction
-const preferenceKeys = Object.keys(DEFAULT_PREFERENCES) as (keyof LocationViewPreferences)[];
+const preferenceKeys: (keyof LocationViewPreferences)[] = [
+  "showDetails",
+  "showEmpty",
+  "editorAdvancedView",
+  "itemDisplayView",
+  "theme",
+  "itemsPerTablePage",
+  "tableHeaders",
+  "displayLegacyHeader",
+  "legacyImageFit",
+  "language",
+  "overrideFormatLocale",
+  "collectionId",
+  "duplicateSettings",
+  "shownMultiTabWarning",
+  "quickActions",
+];
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const preferenceKeys = Object.keys(DEFAULT_PREFERENCES) as (keyof LocationViewPreferences)[];
export function forEachSyncedPreference(
syncConfig: PreferenceSyncConfig,
callback: (key: keyof LocationViewPreferences) => void
) {
for (const key of preferenceKeys) {
if (syncConfig[key] !== false) {
callback(key);
}
}
}
export function buildSyncedSettings(
preferences: LocationViewPreferences,
syncConfig: PreferenceSyncConfig
): Record<string, unknown> {
const payload: Record<string, unknown> = {};
forEachSyncedPreference(syncConfig, key => {
payload[key] = preferences[key];
});
return payload;
}
export function mergeSyncedSettings(
settings: Record<string, unknown>,
preferences: LocationViewPreferences,
syncConfig: PreferenceSyncConfig
): LocationViewPreferences {
const nextPreferences = { ...preferences };
forEachSyncedPreference(syncConfig, key => {
if (key in settings) {
nextPreferences[key] = settings[key] as never;
}
});
return nextPreferences;
const preferenceKeys: (keyof LocationViewPreferences)[] = [
"showDetails",
"showEmpty",
"editorAdvancedView",
"itemDisplayView",
"theme",
"itemsPerTablePage",
"tableHeaders",
"displayLegacyHeader",
"legacyImageFit",
"language",
"overrideFormatLocale",
"collectionId",
"duplicateSettings",
"shownMultiTabWarning",
"quickActions",
];
export function forEachSyncedPreference(
syncConfig: PreferenceSyncConfig,
callback: (key: keyof LocationViewPreferences) => void
) {
for (const key of preferenceKeys) {
if (syncConfig[key] !== false) {
callback(key);
}
}
}
export function buildSyncedSettings(
preferences: LocationViewPreferences,
syncConfig: PreferenceSyncConfig
): Record<string, unknown> {
const payload: Record<string, unknown> = {};
forEachSyncedPreference(syncConfig, key => {
payload[key] = preferences[key];
});
return payload;
}
export function mergeSyncedSettings(
settings: Record<string, unknown>,
preferences: LocationViewPreferences,
syncConfig: PreferenceSyncConfig
): LocationViewPreferences {
const nextPreferences = { ...preferences };
forEachSyncedPreference(syncConfig, key => {
if (key in settings) {
nextPreferences[key] = settings[key] as never;
}
});
return nextPreferences;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/composables/preferences-utils.ts` around lines 65 - 100, The synced
preference key list is incorrectly derived from DEFAULT_PREFERENCES, so optional
LocationViewPreferences fields like tableHeaders and collectionId are skipped
during serialization and merge. Update preferences-utils.ts to source the
syncable keys from the full LocationViewPreferences shape or an explicit shared
key list instead of DEFAULT_PREFERENCES, and make forEachSyncedPreference,
buildSyncedSettings, and mergeSyncedSettings operate over that complete set so
enabled optional preferences are preserved.

Comment thread frontend/composables/use-preferences.ts Outdated
Comment thread frontend/lib/integration-adapters.ts Outdated
Comment on lines +50 to +65
if (baseUrl?.trim()) {
try {
const base = new URL(baseUrl.trim());
if (base.origin !== target.origin) return null;
basePath = base.pathname.replace(/\/$/, "");
if (basePath && !target.pathname.startsWith(basePath + "/") && target.pathname !== basePath) {
return null;
}
} catch (e) {
// Invalid configured base URL – fall through to pattern-only match.
console.warn("integration-adapters: invalid baseUrl, falling back to pattern-only match:", baseUrl, e);
}
}

const pathAfterBase = target.pathname.slice(basePath.length || 0);
return pathAfterBase.match(pattern)?.[1] ?? null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Do not treat an invalid configured base URL as a match.

When baseUrl is supplied but unparseable, this helper still falls back to pattern-only extraction. That makes extractId() promote any /documents/{id} link when the saved Paperless URL is malformed, even though detectServiceFromUrl() now intentionally returns null unless the configured base URL parses and matches. Return null once a configured URL is present but invalid, and keep the heuristic fallback only for the baseUrl === undefined case.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/lib/integration-adapters.ts` around lines 50 - 65, In
integration-adapters.ts, the helper that computes the extracted ID currently
falls back to pattern-only matching even when a configured baseUrl is present
but invalid, which can incorrectly accept links; update the logic around the URL
parsing block and the path extraction in this function so that any non-empty
baseUrl that fails to parse returns null immediately, while preserving the
existing heuristic pattern-only fallback only when baseUrl is undefined. Use the
existing URL matching flow in this helper (including target.origin, basePath,
and the final pathAfterBase.match call) to keep the valid configured-base
behavior unchanged.

Comment thread frontend/pages/reports/label-generator.vue Outdated
Comment thread frontend/stores/integration-cache.ts Outdated
Comment on lines +110 to +128
function getEnrichedData(serviceName: string, id: string): unknown {
const key = `${serviceName}:${id}`;
if (key in enrichedData) return enrichedData[key];
const cached = lsRead(key);
if (cached !== null) enrichedData[key] = cached;
return cached;
}

function setEnrichedData(serviceName: string, id: string, data: unknown): void {
const key = `${serviceName}:${id}`;
enrichedData[key] = data;
lsWrite(key, data);
}

function invalidateEnrichedData(serviceName: string, id: string): void {
const key = `${serviceName}:${id}`;
Reflect.deleteProperty(enrichedData, key);
lsDelete(key);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Scope cached enrichment data to the user and the configured service URL.

The cache key is only ${serviceName}:${id}. Because this store persists that data in localStorage, a second Homebox account on the same browser profile—or the same account after repointing paperless_url at a different Paperless instance—can reuse the previous title/tags for the same document id. If the new service is unreachable, that stale metadata survives for the full TTL. Please namespace the cache with at least the authenticated user id and normalized base URL, or clear the service cache whenever setServiceUrl() changes.

Also applies to: 148-150

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/stores/integration-cache.ts` around lines 110 - 128, The enrichment
cache key is too broad because getEnrichedData, setEnrichedData, and
invalidateEnrichedData only namespace by serviceName and id, so persisted
localStorage entries can leak across users or different Paperless URLs. Update
the cache keying in integration-cache.ts to include the authenticated user id
and a normalized service base URL, or alternatively clear all enrichment entries
when setServiceUrl() changes to a new backend. Make sure the same
namespacing/clear behavior is applied consistently in the read, write, and
invalidate paths so stale metadata cannot be reused across accounts or service
instances.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
frontend/pages/item/[id]/index/edit.vue (2)

558-571: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Defaulting entityTypeId to an empty string risks corrupting the entity's type on save.

entityTypeId: item.value.entityType?.id || "" will submit an empty string to api.items.update if entityType is ever missing, whereas saveItem treats this field as required (item.value.entityType!.id). Sending "" could either fail validation or, worse, silently clear/break the entity-type association during a location sync — a data-integrity risk distinct from a simple no-op.

Consider guarding and aborting (similar to the existing !item.value.parent?.id check just above) instead of silently defaulting:

🛡️ Suggested fix
+    if (!item.value.entityType?.id) {
+      toast.error(t("items.toast.failed_save"));
+      return;
+    }
     const payload: EntityUpdate = {
       ...item.value,
       parentId: parent.value?.id || item.value.parent?.id || null,
       tagIds: item.value.tagIds,
       assetId: item.value.assetId,
-      entityTypeId: item.value.entityType?.id || "",
+      entityTypeId: item.value.entityType.id,
       syncChildEntityLocations: item.value.syncChildEntityLocations,
     };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/pages/item/`[id]/index/edit.vue around lines 558 - 571, The
syncChildEntityLocations flow is defaulting entityTypeId to an empty string,
which can corrupt the item’s type on update. In syncChildEntityLocations,
replace the fallback on item.value.entityType?.id with a required guard like the
existing parent/location check, and abort with a toast or error if entityType is
missing before calling api.items.update. Keep the payload construction in sync
with saveItem by ensuring entityTypeId is only sent when item.value.entityType
is present.

110-120: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove leftover debug console.log statements.

Lines 119-120 log purchasePrice/soldPrice to the console via a redundant ??= mutation — purchasePrice/soldPrice are already correctly computed just above (lines 110-117). This looks like a debugging leftover; it's dead logic and needlessly surfaces monetary data in the browser console.

🧹 Suggested fix
-    console.log((item.value.purchasePrice ??= 0));
-    console.log((item.value.soldPrice ??= 0));
-
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/pages/item/`[id]/index/edit.vue around lines 110 - 120, Remove the
leftover debug console output in the item edit page: the `console.log` calls in
the `index/edit.vue` logic are redundant because `purchasePrice` and `soldPrice`
are already computed above. Delete the two `console.log` statements and avoid
using the `??=` mutations there, keeping the existing price calculation flow in
that block unchanged.
🧹 Nitpick comments (5)
frontend/lib/preferences-utils.ts (2)

65-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Manual key list can drift from LocationViewPreferences/DEFAULT_PREFERENCES.

preferenceKeys is now hand-maintained instead of derived from Object.keys(DEFAULT_PREFERENCES). If a new field is later added to LocationViewPreferences/DEFAULT_PREFERENCES but this array is forgotten, it will silently stop syncing (no compile error since it's just a subset list).

♻️ Suggested guard to keep the list in sync
-const preferenceKeys: (keyof LocationViewPreferences)[] = [
-  "showDetails",
-  "showEmpty",
-  "editorAdvancedView",
-  "itemDisplayView",
-  "theme",
-  "itemsPerTablePage",
-  "tableHeaders",
-  "displayLegacyHeader",
-  "legacyImageFit",
-  "language",
-  "overrideFormatLocale",
-  "collectionId",
-  "duplicateSettings",
-  "shownMultiTabWarning",
-  "quickActions",
-];
+const preferenceKeys: (keyof LocationViewPreferences)[] = [
+  "showDetails",
+  "showEmpty",
+  "editorAdvancedView",
+  "itemDisplayView",
+  "theme",
+  "itemsPerTablePage",
+  "tableHeaders",
+  "displayLegacyHeader",
+  "legacyImageFit",
+  "language",
+  "overrideFormatLocale",
+  "collectionId",
+  "duplicateSettings",
+  "shownMultiTabWarning",
+  "quickActions",
+];
+
+// Compile-time guard: fails the build if a key exists in DEFAULT_PREFERENCES
+// but is missing from preferenceKeys (or vice versa).
+type _AssertPreferenceKeysComplete = Exclude<keyof LocationViewPreferences, (typeof preferenceKeys)[number]> extends never
+  ? true
+  : false & { error: "preferenceKeys is missing a key from LocationViewPreferences" };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/lib/preferences-utils.ts` around lines 65 - 81, The manually
maintained preferenceKeys list can drift from
LocationViewPreferences/DEFAULT_PREFERENCES and silently miss new settings.
Update the logic in preferences-utils.ts so preferenceKeys is derived from
DEFAULT_PREFERENCES (or another single source of truth) instead of a hardcoded
subset, and keep the existing sync behavior in the helper that uses
preferenceKeys so newly added preference fields are included automatically.

105-119: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Security note: allowlist is good, but consider validating value shapes too.

Restricting assignment to keys from the fixed preferenceKeys array (rather than iterating settings's own keys) is a solid defense against prototype-pollution-style key injection from server JSON — good call. That said, the values themselves (e.g. duplicateSettings, quickActions, tableHeaders) are cast with as never and copied without shape/type validation. A malformed or unexpected value type from the server could propagate into app state and cause runtime errors in consumers that assume the well-typed shape.

Consider adding lightweight runtime validation (e.g. typeof/shape checks or a schema validator) for structurally complex keys before assignment, as defense-in-depth against unexpected server payloads.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/lib/preferences-utils.ts` around lines 105 - 119, The
mergeSyncedSettings function currently allowlists keys via
forEachSyncedPreference, but it still copies server-provided values directly
into nextPreferences with only a cast, so malformed shapes can flow into app
state. Add lightweight runtime validation before assigning each synced value,
especially for structurally complex preference keys like duplicateSettings,
quickActions, and tableHeaders. Keep the existing key allowlist, but gate
assignment on basic typeof/shape checks or a small schema helper so only
expected value shapes are merged.
backend/app/api/handlers/v1/v1_ctrl_integration_cards.go (1)

100-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate URL-validation helper.

parseHTTPURL re-implements the same scheme/host/no-userinfo checks as parseExternalHTTPURL in v1_ctrl_entities_attachments_external.go. Both enforce the same security invariant (must be http/https, no embedded credentials); consolidating into one shared helper avoids the two drifting apart over time.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards.go` around lines 100 -
112, The URL validation logic in parseHTTPURL duplicates the same http/https,
host, and no-userinfo checks already implemented in parseExternalHTTPURL, so
consolidate them into a single shared helper. Update parseHTTPURL to call the
existing shared validation path (or extract a common helper used by both
parseHTTPURL and parseExternalHTTPURL) so the security invariant stays
consistent and avoids future drift.
frontend/components/Item/AttachmentsList.vue (2)

43-52: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Security note: tag color/textColor are bound as raw inline styles from an external source.

tag.color/tag.textColor originate from the Paperless API response and flow straight into :style. While Vue's binding limits this to CSS (no script execution), unsanitized values could still enable minor CSS-based data exfiltration tricks. Consider validating the values look like colors (e.g., a hex/rgb() regex) before binding.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/components/Item/AttachmentsList.vue` around lines 43 - 52, The tag
rendering in AttachmentsList.vue binds tag.color and tag.textColor directly into
inline styles, so validate/sanitize these values before passing them to :style.
Update the cardTags(attachment) usage or add a small helper in the
AttachmentsList component to accept only safe color formats (for example, hex or
rgb/rgba) and fall back to default styling when values are invalid.

63-90: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Consider validating openUrl before binding to href, mirroring the safeLinkURL guard used below.

integrationCard(attachment)?.openUrl is bound directly into an anchor href with target="_blank". It's server-generated today, but this component previously required the same guard for attachment.path on the exact same security grounds (untrusted scheme → active link). As integrations expand (e.g., Immich per the PR roadmap), a defense-in-depth check here would prevent a future provider bug from becoming a client-side issue.

🔒 Suggested guard
-                    :href="integrationCard(attachment)?.openUrl"
+                    :href="safeOpenURL(integrationCard(attachment))"

And reuse (or generalize) the existing URL-validation logic used for safeLinkURL.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/components/Item/AttachmentsList.vue` around lines 63 - 90, The
open-in-new-tab anchor in AttachmentsList.vue binds
integrationCard(attachment)?.openUrl directly to href without the same safety
check used for attachment.path. Update the rendering around the TooltipTrigger/a
link to validate openUrl with the existing safeLinkURL logic (or a
shared/generalized helper) before assigning it to href, and only render the
active link when the URL passes validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards_test.go`:
- Around line 14-63: The test table in v1_ctrl_integration_cards_test.go repeats
the same base URL literal multiple times, triggering goconst. Introduce a
package-level test constant for the shared Paperless base URL and update the
affected cases in the integration card matching tests to use it. Keep the
existing symbols like the table-driven test cases in the v1 controller
integration card tests so the new constant is easy to apply across the repeated
entries.

In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards.go`:
- Around line 204-317: The card-building path is doing many sequential upstream
Paperless requests in buildPaperlessCard and
HandleEntityAttachmentIntegrationCards, which can make the endpoint stall on
entities with multiple tagged documents. Refactor buildPaperlessCard to fetch
the optional correspondent, documentType, and tag details concurrently, and add
request-scoped caching for repeated IDs so the same lookups are not repeated
across cards. Also ensure the overall handler uses a shared deadline or timeout
for the full card-building pass so one slow attachment cannot block the entire
response.
- Around line 373-376: The thumbnail response in the integration card handler is
blindly forwarding the upstream Content-Type from
resp.Header.Get("Content-Type"), which can expose this authenticated endpoint to
non-image content from a user-configurable paperless_url. Update the response
handling in the thumbnail path (around the io.Copy(w, resp.Body) flow) to only
pass through an allow-list of safe image MIME types and otherwise fall back to a
safe default or omit the header, and add X-Content-Type-Options: nosniff before
writing the body.
- Around line 87-98: The Paperless integration is still inheriting the shared
outbound policy, which leaves RFC1918 and localhost destinations reachable by
default. Update paperlessConfigFromSettings (and any Paperless URL validation it
relies on) to explicitly reject local-network and localhost targets for
paperless_url unless the relevant HBOX_NOTIFIER_BLOCK_LOCAL_NETS and
HBOX_NOTIFIER_BLOCK_LOCALHOST protections are enabled, so authenticated users
cannot point this integration at internal services.

In `@frontend/components/Item/AttachmentsList.vue`:
- Around line 205-214: `loadIntegrationCards` in `AttachmentsList.vue` can apply
out-of-order results when multiple `watch(attachmentSignature, ...)` updates
overlap. Add a latest-request guard in `loadIntegrationCards` (or around the
`api.items.attachments.integrationCards` call) so only the most recent fetch is
allowed to assign to `cards.value`, and ignore stale responses from earlier
requests.

In `@frontend/pages/profile.vue`:
- Around line 253-256: The settings loader in profile.vue is still using unsafe
string casts for paperless_url and paperless_token, so mirror the hardened
pattern already used in edit.vue by checking the runtime type before assignment.
Update the logic around integrationSettings.paperless_url and
integrationSettings.paperless_token to only accept values when typeof is
"string", and otherwise fall back to an empty string. Use the existing settings
object in this block to keep the fix localized and consistent with the other
loader.

---

Outside diff comments:
In `@frontend/pages/item/`[id]/index/edit.vue:
- Around line 558-571: The syncChildEntityLocations flow is defaulting
entityTypeId to an empty string, which can corrupt the item’s type on update. In
syncChildEntityLocations, replace the fallback on item.value.entityType?.id with
a required guard like the existing parent/location check, and abort with a toast
or error if entityType is missing before calling api.items.update. Keep the
payload construction in sync with saveItem by ensuring entityTypeId is only sent
when item.value.entityType is present.
- Around line 110-120: Remove the leftover debug console output in the item edit
page: the `console.log` calls in the `index/edit.vue` logic are redundant
because `purchasePrice` and `soldPrice` are already computed above. Delete the
two `console.log` statements and avoid using the `??=` mutations there, keeping
the existing price calculation flow in that block unchanged.

---

Nitpick comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards.go`:
- Around line 100-112: The URL validation logic in parseHTTPURL duplicates the
same http/https, host, and no-userinfo checks already implemented in
parseExternalHTTPURL, so consolidate them into a single shared helper. Update
parseHTTPURL to call the existing shared validation path (or extract a common
helper used by both parseHTTPURL and parseExternalHTTPURL) so the security
invariant stays consistent and avoids future drift.

In `@frontend/components/Item/AttachmentsList.vue`:
- Around line 43-52: The tag rendering in AttachmentsList.vue binds tag.color
and tag.textColor directly into inline styles, so validate/sanitize these values
before passing them to :style. Update the cardTags(attachment) usage or add a
small helper in the AttachmentsList component to accept only safe color formats
(for example, hex or rgb/rgba) and fall back to default styling when values are
invalid.
- Around line 63-90: The open-in-new-tab anchor in AttachmentsList.vue binds
integrationCard(attachment)?.openUrl directly to href without the same safety
check used for attachment.path. Update the rendering around the TooltipTrigger/a
link to validate openUrl with the existing safeLinkURL logic (or a
shared/generalized helper) before assigning it to href, and only render the
active link when the URL passes validation.

In `@frontend/lib/preferences-utils.ts`:
- Around line 65-81: The manually maintained preferenceKeys list can drift from
LocationViewPreferences/DEFAULT_PREFERENCES and silently miss new settings.
Update the logic in preferences-utils.ts so preferenceKeys is derived from
DEFAULT_PREFERENCES (or another single source of truth) instead of a hardcoded
subset, and keep the existing sync behavior in the helper that uses
preferenceKeys so newly added preference fields are included automatically.
- Around line 105-119: The mergeSyncedSettings function currently allowlists
keys via forEachSyncedPreference, but it still copies server-provided values
directly into nextPreferences with only a cast, so malformed shapes can flow
into app state. Add lightweight runtime validation before assigning each synced
value, especially for structurally complex preference keys like
duplicateSettings, quickActions, and tableHeaders. Keep the existing key
allowlist, but gate assignment on basic typeof/shape checks or a small schema
helper so only expected value shapes are merged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4a0f01d2-8e6a-49bd-9044-81a7d299617d

📥 Commits

Reviewing files that changed from the base of the PR and between 51a4f99 and 823250b.

📒 Files selected for processing (25)
  • backend/app/api/handlers/v1/controller.go
  • backend/app/api/handlers/v1/v1_ctrl_entities_attachments_external.go
  • backend/app/api/handlers/v1/v1_ctrl_integration_cards.go
  • backend/app/api/handlers/v1/v1_ctrl_integration_cards_test.go
  • backend/app/api/routes.go
  • backend/internal/core/services/service_items_attachments.go
  • backend/internal/core/services/service_items_attachments_external_test.go
  • backend/internal/core/services/service_user.go
  • backend/internal/data/repo/repo_item_attachments.go
  • backend/internal/data/repo/repo_item_attachments_test.go
  • frontend/components/Entity/CreateModal.vue
  • frontend/components/Item/AttachmentsList.vue
  • frontend/components/Item/View/ItemChangeDetails.vue
  • frontend/components/Item/View/table/data-table-dropdown.vue
  • frontend/composables/use-preferences.test.ts
  • frontend/composables/use-preferences.ts
  • frontend/lib/api/base/base-api.ts
  • frontend/lib/api/classes/items.ts
  • frontend/lib/preferences-utils.ts
  • frontend/locales/de.json
  • frontend/locales/en.json
  • frontend/pages/item/[id]/index.vue
  • frontend/pages/item/[id]/index/edit.vue
  • frontend/pages/profile.vue
  • frontend/pages/reports/label-generator.vue
✅ Files skipped from review due to trivial changes (2)
  • frontend/lib/api/base/base-api.ts
  • frontend/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (9)
  • frontend/components/Item/View/ItemChangeDetails.vue
  • frontend/composables/use-preferences.test.ts
  • frontend/pages/reports/label-generator.vue
  • frontend/components/Entity/CreateModal.vue
  • frontend/pages/item/[id]/index.vue
  • frontend/composables/use-preferences.ts
  • backend/internal/data/repo/repo_item_attachments_test.go
  • backend/internal/core/services/service_user.go
  • backend/internal/core/services/service_items_attachments_external_test.go

Comment thread backend/app/api/handlers/v1/v1_ctrl_integration_cards_test.go
Comment thread backend/app/api/handlers/v1/v1_ctrl_integration_cards.go
Comment thread backend/app/api/handlers/v1/v1_ctrl_integration_cards.go Outdated
Comment thread backend/app/api/handlers/v1/v1_ctrl_integration_cards.go Outdated
Comment thread frontend/components/Item/AttachmentsList.vue
Comment thread frontend/pages/profile.vue Outdated
@coderabbitai coderabbitai Bot added the 🕷️ bug Something isn't working label Jul 2, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards_test.go`:
- Around line 132-136: The test table in v1_ctrl_integration_cards_test.go still
hardcodes the fallback MIME type literal instead of reusing the extracted
thumbnail MIME constants. Update the cases in the integration cards test table
to reference the same production constants used by the thumbnail MIME handling
logic, especially the default/fallback value, so the expectations stay aligned
with the symbols that define MIME normalization.

In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards.go`:
- Around line 82-90: The thumbnail handling in v1_ctrl_integration_cards is
duplicating the MIME literals "application/octet-stream" and "image/png", which
triggers goconst. Introduce shared constants for the thumbnail fallback and
allowed MIME values near the existing integration card helpers (for example
alongside integrationCardBuildTimeout and newPaperlessCardCache), then update
the relevant thumbnail logic and the matching tests to reference those constants
instead of repeating string literals. Use the existing thumbnail-related
functions and helpers in this file to locate all repeated checks and fallback
assignments.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 9e6903fd-6424-41cb-af61-e92b06becb5e

📥 Commits

Reviewing files that changed from the base of the PR and between 9646d5b and a5592da.

📒 Files selected for processing (4)
  • backend/app/api/handlers/v1/v1_ctrl_integration_cards.go
  • backend/app/api/handlers/v1/v1_ctrl_integration_cards_test.go
  • frontend/components/Item/AttachmentsList.vue
  • frontend/pages/profile.vue
🚧 Files skipped from review as they are similar to previous changes (2)
  • frontend/pages/profile.vue
  • frontend/components/Item/AttachmentsList.vue

Comment thread backend/app/api/handlers/v1/v1_ctrl_integration_cards_test.go Outdated
Comment on lines +82 to +90
const integrationCardBuildTimeout = 15 * time.Second

func newPaperlessCardCache() *paperlessCardCache {
return &paperlessCardCache{
correspondents: make(map[int]cachedIntegrationLabel),
documentTypes: make(map[int]cachedIntegrationLabel),
tags: make(map[int]cachedIntegrationTag),
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Extract thumbnail MIME literals to constants to keep CI green.

goconst is failing on repeated "application/octet-stream" and "image/png" literals. Centralize the thumbnail fallback and allow-list values, then reuse them in the tests.

🔧 Proposed fix
-const integrationCardBuildTimeout = 15 * time.Second
+const (
+	integrationCardBuildTimeout = 15 * time.Second
+
+	thumbnailFallbackContentType = "application/octet-stream"
+	thumbnailAVIFContentType     = "image/avif"
+	thumbnailBMPContentType      = "image/bmp"
+	thumbnailGIFContentType      = "image/gif"
+	thumbnailJPEGContentType     = "image/jpeg"
+	thumbnailPNGContentType      = "image/png"
+	thumbnailWebPContentType     = "image/webp"
+)
 func thumbnailContentType(contentType string) string {
 	mediaType, _, err := mime.ParseMediaType(contentType)
 	if err != nil {
-		return "application/octet-stream"
+		return thumbnailFallbackContentType
 	}

 	switch strings.ToLower(mediaType) {
-	case "image/avif", "image/bmp", "image/gif", "image/jpeg", "image/png", "image/webp":
+	case thumbnailAVIFContentType, thumbnailBMPContentType, thumbnailGIFContentType, thumbnailJPEGContentType, thumbnailPNGContentType, thumbnailWebPContentType:
 		return mediaType
 	default:
-		return "application/octet-stream"
+		return thumbnailFallbackContentType
 	}
 }

Also applies to: 379-390

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards.go` around lines 82 -
90, The thumbnail handling in v1_ctrl_integration_cards is duplicating the MIME
literals "application/octet-stream" and "image/png", which triggers goconst.
Introduce shared constants for the thumbnail fallback and allowed MIME values
near the existing integration card helpers (for example alongside
integrationCardBuildTimeout and newPaperlessCardCache), then update the relevant
thumbnail logic and the matching tests to reference those constants instead of
repeating string literals. Use the existing thumbnail-related functions and
helpers in this file to locate all repeated checks and fallback assignments.

Source: Linters/SAST tools

@coderabbitai coderabbitai Bot removed the 🕷️ bug Something isn't working label Jul 2, 2026
@szaiser

szaiser commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⬆️ enhancement New feature or request go Pull requests that update Go code review needed A review is needed on this PR or Issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants